Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jodis should return jedis resource after using it #166

Closed
tobegit3hub opened this issue Mar 26, 2015 · 7 comments
Closed

Jodis should return jedis resource after using it #166

tobegit3hub opened this issue Mar 26, 2015 · 7 comments

Comments

@tobegit3hub
Copy link
Contributor

Refer to official document redis/jedis#476 , jedis object is not thread-safe and we should return the resource after using it from pool.

Jedis jedis = pool.getResource();
try {
  /// ... do stuff here ... for example
  jedis.set("foo", "bar");
  String foobar = jedis.get("foo");
  jedis.zadd("sose", 0, "car"); jedis.zadd("sose", 0, "bike"); 
  Set<String> sose = jedis.zrange("sose", 0, -1);
} catch (JedisConnectionException e) {
    // returnBrokenResource when the state of the object is unrecoverable
    if (null != jedis) {
        pool.returnBrokenResource(jedis);
        jedis = null;
    }
} finally {
  /// ... it's important to return the Jedis instance to the pool once you've finished using it
  if (null != jedis)
    pool.returnResource(jedis);
}

This is the proper way to use jedis pool. I think jodis provide the interface to get resource from pool but no way to return the resource.

@ngaut
Copy link
Contributor

ngaut commented Mar 26, 2015

JedisResourcePool jedisPool = new RoundRobinJedisPool("zkserver:2181", 30000, "/zk/codis/db_xxx/proxy", new JedisPoolConfig());
try (Jedis jedis = jedisPool.getResource()) {
jedis.set("foo", "bar");
String value = jedis.get("foo");
}

After the try block, jodis will rerurn it to pool automaticly

@tobegit3hub
Copy link
Contributor Author

I have read the code and found no code about returnResource or returnBrokenResource. Refer to https://github.com/wandoulabs/codis/blob/cc90bb50d9dc6cda85c6ea0b54eb1227a41a5350/extern/jodis/src/main/java/com/wandoulabs/jodis/JedisResourcePool.java#L41, jodis ask users to close jedis instance by themselves.

However, calling jedis.close() is not good enough and doesn't help to return the resources to pool. Please refer to redis/jedis#562 and redis/jedis#500.

@c4pt0r
Copy link
Contributor

c4pt0r commented Mar 27, 2015

https://github.com/xetorthio/jedis/blob/master/src/main/java/redis/clients/jedis/Jedis.java#L3166
shows that when closing jedis instance, it will return itself to its datasource(in our case, it's a JedisPool which created by Jodis).

jodis will not change the original behavior of JedisPool.

@tobegit3hub
Copy link
Contributor Author

Thanks @c4pt0r . You're right and I hope you to read this through 😃

The latest version of jedis(currently 3.0.0-SNAPSHOT) has fixed this problem after redis/jedis#909. If we use 3.0.0, we can just close the jedis instances without returning resources to the pool. You can know about this from the commit message.

commit 6701b07904357dd759a11209ce98b71bd398b0fd
Refs: jedis-2.5.2-272-g6701b07
Author:     Marcos Lilljedahl <[email protected]>
AuthorDate: Mon Mar 9 02:33:50 2015 -0400
Commit:     Marcos Lilljedahl <[email protected]>
CommitDate: Mon Mar 9 02:33:50 2015 -0400

    This commit addresses the first part discussed in #909.
    To improve usability we're changing the visibility of `returnResoure`
    and `returnBrokenResource` in favor of `jedis.close()` which
    automatically returns the resource to the pool accordingly.
    As `Pool` and `Jedis` are currently in different packages i've created
    a JedisPoolAbstract class to provide a bridge between the two
    implementations

This is merged in March 9th, 2015. Before this day, all the usages without calling returnResource() or returnBrokenResource() may result in memory leak. If jodis wants to take this advantage, you should dependent on jedis:3.0.0-SNATSHOP or wait for the stable release.

You're lucky to get the right result but the current usage is totally wrong. I wanna make sure you know about that.

BTW, redis/jedis#562 has rejected to use close to recycle resources because it breaks the backward compatible. That's why they're making it 3.0.0.

@tobegit3hub
Copy link
Contributor Author

It seems I misunderstand the commit message. The https://github.com/xetorthio/jedis/releases/tag/jedis-2.6.0 has supported TryWithResource syntax and it would be okay to use it after 2.6.0. The problem of 3.0.0 is that we can't use pool to call returnResource or returnBrokenResource. This breaks the backward compatible and we shouldn't use them after 3.0.0.

Thanks for your support @Apache9 😃

@Apache9
Copy link
Member

Apache9 commented Mar 29, 2015

My pleasure.

@jerry-024
Copy link

need not return resource?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

5 participants