Jedis.close() should return Jedis to pool instead of closing it

[secondorderpolynomial]

What steps will reproduce the problem?

1. try (Jedis jedis = pool.getResource()) {}

What is the expected output? What do you see instead?

Jedis was accessed from a pool. Jedis should be returned to the pool. Instead the connection is closed.
This makes it impossible to use Jedis in a try-with-resource without closing it.
Also, findbugs complains if close isn't called on Jedis, as it is a Closable.

What version of the product are you using? On what operating system?

2.4.0
OS: irrelevant

Please provide any additional information below.

I think it's stylistically terse to use Jedis with a try-with-resource. This behavior would be similar to JDBC DataSource connection pooling, and I am assuming, easy enough to implement.

[xetorthio]

Mmm... this is tricky.

I wonder if flagging the Jedis instance, when it is obtained from a pool, enough to know how to deal with it when calling close().

[secondorderpolynomial]

How about wrapping it?

[HeartSaVioR]

I'm planning to implement PooledJedis wrapping Jedis class and provide close().
It's already discussed from closed issue, #500 . (Thanks again @andekaputra!)
See #500 (comment) for further information.

[secondorderpolynomial]

Great. Sounds right to me.

[HeartSaVioR]

Jedis and BinaryJedis is easy to implement close() because it just have to call disconnect().
But Pooled Jedis instance is somewhat not easy because of two methods of returns.
(returnResource() and returnBrokenResource())
So instance has to keep track of connection status (maybe broken or not) and determine what method should he call.

[HeartSaVioR]

@secondorderpolynomial Thanks for triple check!
I think @andekaputra is right, but I was focusing on other issues and got lost.
Since Jedis/BinaryJedis supports try-with-resource and it may lead side-effect you reported, It's time to implement these things!

[xetorthio]

I missed the discussion around this.
But I feel like maybe a wrapper is not needed. Why tagging the instance is
not enough?

On Fri, Feb 21, 2014 at 6:03 PM, Jungtaek Lim [email protected]:

@secondorderpolynomial https://github.com/secondorderpolynomial Thanks
for triple check!
I think @andekaputra https://github.com/andekaputra is right, but I was
focusing on other issues and got lost.
Since Jedis/BinaryJedis supports try-with-resource and it may lead
side-effect you reported, It's time to implement these things!

Reply to this email directly or view it on GitHubhttps://github.com//issues/562#issuecomment-35783113
.

[HeartSaVioR]

@xetorthio Jedis needs to keep track of pool instance (their source) to return itself to pool.
So Jedis instance needs two fields, pool, broken, while it doesn't need to Non-pooled Jedis instances.

And I will try to implement PooledJedisCommand, same thing to JedisClusterCommand.
(JedisClusterCommand is amazing! I'm so inspired.)

It makes us easy to implement new things should applied to all commands.
"checking broken status" (just catch JedisConnectionException and mark, and re-throw) is a good example. :)

[xetorthio]

Need to see hpw the usage will end up.
Are you thinking of a PooledJedis that extends Jedis and adds those 2
fields?
On Feb 21, 2014 6:28 PM, "Jungtaek Lim" [email protected] wrote:

@xetorthio https://github.com/xetorthio Jedis needs to keep track of
pool instance (their source) to return itself to pool.
So Jedis instance needs two fields, pool, broken, while it doesn't need to
Non-pooled Jedis instances.

And I will try to implement PooledJedisCommand, same thing to
JedisClusterCommand.
(JedisClusterCommand is amazing! I'm so inspired.)

It makes us easy to implement new things should applied to all commands.
"checking broken status" (just catch JedisConnectionException and mark,
and re-throw) is a good example. :)

Reply to this email directly or view it on GitHubhttps://github.com//issues/562#issuecomment-35784748
.

[secondorderpolynomial]

@xetorthio I think it would be right for Jedis to NOT have the notion of whether or not it is a pooled resource.

@HeartSaVioR Valid point about returnResource vs. returnBrokenResource. Is it always possible for PooledJedis/PooledJedisCommand to accurately determine if a connection is broken, or would it require the user (of the code) to mark Jedis as broken?

Praveen

[xetorthio]

The thing here is the usage, impact on the api and impact on the code.
I always tried to keep jedis simple and minimal. In java it is very easy to
start adding layers on top of layers, etc.
In my opinion adding a PooledJedis and PooledJedisCommand needs to prove to
be really superior to adding 2 fields in Jedis class.
I am not saying it is not, I just don't see it clear yet.
Also the change you propose is not backward compatible, which is fine, but
it is something to have in mind.
On Feb 21, 2014 7:43 PM, "secondorderpolynomial" [email protected]
wrote:

@xetorthio https://github.com/xetorthio I think it would be right for
Jedis to NOT have the notion of whether or not it is a pooled resource.

@HeartSaVioR https://github.com/HeartSaVioR Valid point about
returnResource vs. returnBrokenResource. Is it always possible for
PooledJedis/PooledJedisCommand to accurately determine if a connection is
broken, or would it require the user (of the code) to mark Jedis as broken?

Praveen

Reply to this email directly or view it on GitHubhttps://github.com//issues/562#issuecomment-35788689
.

[HeartSaVioR]

@secondorderpolynomial
Based on current Jedis*Pool usage, Jedis itself can know he's broken by intercept JedisConnectionException from all commands.
So I think users don't need to mark broken status by hand, and for convenience user shouldn't.

@xetorthio I agree @secondorderpolynomial .
It's easier to implement if we add two fields to Jedis, but it's not beautiful because of dependency.
I don't like cross-reference so I don't like this approach.
But it's mandatory to make close() work, so I wish to minimize impact about cross-reference.

And PooledJedis would 100% compatible to Jedis (should make it).
So users don't need to know Jedis instance type is PooledJedis or not.
Then I think we can return PooledJedis by traditional method.(try, catch, finally) and finally we accomplish backward compatible.

All things are not 100% sure, so I wish to try implementing on it without talk to others, but I finally couldn't. ;)
If you find something wrong or curious, let me know about it. :)

Thank you all!

[HeartSaVioR]

@xetorthio I missed to reply your comment.
Just flagging "it's pooled" is not enough, because Jedis.close() would automatically called by VM, and Jedis is responsible to returning itself pool by itself, it cannot lend other's hand.
I think it's only possible to achieve this by cross-reference, but if you find better approaches, please let me know, I don't like cross-reference.
Thanks!

[HeartSaVioR]

@secondorderpolynomial @xetorthio
After digging several hours, I found out there seems to be unavoidable break compatibility to implement PooledJedis (especially working with Jedis*Pool - generic parameterized), and applying Command Pattern is very annoying because all commands are exposed separately to BinaryJedis, Jedis, PipelineBase.
So I changed my mind to allow flag field & cross-reference, and finally implement it with much reduced time!
Please review #563 and comment! Thanks all!

[andekaputra]

Just a thought, I agree on creating a class PooledJedis that inherits Jedis. It can just override method close() to check connection by calling isConnected() to decide either to call returnResource()/returnBrokenResource(). You also need to add method (e.g.) destroy() to PooledJedis to call its super method close() to actually close the connection.

Therefore, there need not be major change of code in Jedis class or any change for that matter.

Sorry for the late response.

[HeartSaVioR]

@andekaputra I also did think it just needs small changes.
But I realized that there's another wall preventing implement this way.
It's JedisPool / JedisSentinelPool.

It extends generic parameterized and its field (internalPool) is also generic parameterized.
If we implement PooledJedis we should also change JedisPool / JedisSentinelPool to extends Pool<PooledJedis>, internalPool and JedisFactory, too.
JedisPool will disallow returnResource() with Jedis class, so we need to declare reference variable/field with "PooledJedis" type.
It means breaking of backward compatible and usages.

[HeartSaVioR]

Closing resolved issue.

[tobegit3hub]

This issue was closed by won't fix. But notice that #909 and 6701b07 have merged to use close() instead of returnResource() or returnBrokenResource() after 3.0.0-SNAPSHOT.

The latest usage is following. Make sure everybody knows about that 😃

Jedis jedis = null;
try {
  jedis = pool.getResource();
  /// ... 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);
} finally {
  if (jedis != null) {
    jedis.close();
  }
}
/// ... when closing your application:
pool.destroy();

[nykolaslima]

@tobegit3hub You are right. I will open a issue to fix it.

[nykolaslima]

@tobegit3hub done. You can follow it here: #935

[marcosnils]

@tobegit3hub I don't quite understand your comment. Jedis.close() now returns the resource to the pool accordingly (whether the resource is broken or not). This change has been introduced since 2.5.0 to all upcoming Jedis versions.

returnBrokenResource and returnResource will be removed starting from 3.0 as we don't need to maintain them anymore.

[tobegit3hub]

You're right @marcosnils and I misunderstood about the feature of Jedis.close(). We can use Jedis.close() since 2.5.0 and we can't use Pool.returnResource() since 3.0.0.

It took us a few days to know about this. Thanks.

[devesh-sh]

Hi I am very new to Jedis and trying to use it in my application.
I have created a configuration file for it which is as below.

package com.redis.config;

import org.springframework.beans.factory.annotation.Autowired;
import org.springframework.cache.CacheManager;
import org.springframework.cache.annotation.CachingConfigurerSupport;
import org.springframework.cache.annotation.EnableCaching;
import org.springframework.context.annotation.Bean;
import org.springframework.context.annotation.ComponentScan;
import org.springframework.context.annotation.Configuration;
import org.springframework.data.redis.cache.RedisCacheManager;
import org.springframework.data.redis.connection.RedisConnectionFactory;
import org.springframework.data.redis.connection.jedis.JedisConnectionFactory;
import org.springframework.data.redis.core.RedisTemplate;
import org.springframework.data.redis.serializer.GenericToStringSerializer;
import org.springframework.data.redis.serializer.Jackson2JsonRedisSerializer;
import org.springframework.data.redis.serializer.StringRedisSerializer;

import redis.clients.jedis.JedisPoolConfig;

@configuration
@EnableCaching
@componentscan(basePackages = {"com.redhat.obpa.service.proctor"})
public class CacheConfig extends CachingConfigurerSupport {

//static final JedisPoolConfig poolConfig = new JedisPoolConfig();

@bean
public JedisConnectionFactory redisConnectionFactory() {
JedisConnectionFactory redisConnectionFactory = new JedisConnectionFactory();

//JedisPoolConfig poolConfig = new JedisPoolConfig();
/*poolConfig.setMaxTotal(10);
poolConfig.setTestOnBorrow(true);
poolConfig.setTestOnReturn(true);*/

// Defaults
redisConnectionFactory.setUsePool(true);
redisConnectionFactory.setHostName("127.0.0.1");
redisConnectionFactory.setPort(6379);
return redisConnectionFactory;

}

@bean
@Autowired
public RedisTemplate<String, Proctor> redisTemplate(RedisConnectionFactory cf) {
RedisTemplate<String, Proctor> redisTemplate = new RedisTemplate<String, Proctor>();
redisTemplate.setConnectionFactory(cf);
redisTemplate.setKeySerializer(new GenericToStringSerializer<>(Long.class));
redisTemplate.setValueSerializer(new Jackson2JsonRedisSerializer<>(User.class));
redisTemplate.afterPropertiesSet();
return redisTemplate;
}

@bean
public CacheManager cacheManager(RedisTemplate redisTemplate) {
RedisCacheManager cacheManager = new RedisCacheManager(redisTemplate);

// Number of seconds before expiration. Defaults to unlimited (0)
cacheManager.setDefaultExpiration(300);
return cacheManager;

}

}

I am using @Cacheable with the below method signature in my service

@Cacheable(value = "user" , key = "#Id")
public Proctor read(Long Id) throws JedisException {

And below is the controller I am using

@RequestMapping(value = "/getUserDetails" , method=RequestMethod.POST, produces=MediaType.APPLICATION_JSON_VALUE)
public ResponseEntity getUserDetails(@RequestParam("Id") Long Id) {

	User user =null;
	user= userService.readUser(Id);
	user= user.updateUserDetails(user);
	return new ResponseEntity<>(user, HttpStatus.OK);
}

I am getting the below exception at Line user= userService.readUser(Id);
Could not return the resource to the pool] with root cause
java.lang.IllegalStateException: Invalidated object not currently part of this pool

The error occured with the first time only when hitting the url
http://localhost:8080/myapp/getUserDetails?Id=112
However data is geting cached for each Id when the url is hit for the first time but produces the above exception.
The exception does not occur when the frequent calls are made to the above url.

Any help would be appreciable.
thanks in advance.

[marcosnils]

Please ask this question in the mailing list. github is used for design discussions and bugs mainly.

http://groups.google.com/group/jedis_redis