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

GET metohd for keys return OK insted of value for key (or nil) as Redis message #397

Closed
ghost opened this issue Feb 19, 2013 · 9 comments
Closed

Comments

@ghost
Copy link

ghost commented Feb 19, 2013

Calling method get for Jedis object sometimes return a OK message instead of value of the key (or nil when key does not exist). This happen when asking and setting is very intensive on Redis.

Snippet of code used:

String redisData = oJedisClient.get(key);
if (redisData != null) {
        System.out.println(redisData);
}

Sometimes return

OK!

I've fixed with a work around (reconnect each time this happens):

String redisData = oJedisClient.get(key);
if (redisData != null) {
    if(redisData.equals("OK")){
             this.resetRedisConnection();
     }
    redisData = oJedisClient.get(key);
    if(redisData==null){
         return null;
   }
 }

I've also checked low level message using Wireshark and comunication protocol it's correct (response nil or value when asked with a GET for a given key).

@bakavic
Copy link

bakavic commented Nov 11, 2013

@MiroBarsa Were you using a Pipeline just before your get()? I experienced the same problem as you on get(), but I resolved it by correctly using sync() as recommended in the wiki (https://github.com/xetorthio/jedis/wiki/AdvancedUsage#pipelining), as opposed to exec().

Not too sure why exec() for Pipeline exists? I think it's a bit of a gotcha, seeing that Transactions use exec(), but it is not so for Pipeline. Another user in the Google Groups also ran into the same trap - https://groups.google.com/forum/#!topic/jedis_redis/d7CVoCIcCzU

@xetorthio
Copy link
Contributor

exec is a valid redis command.
it is there because you could use transactions inside a pipeline.

On Mon, Nov 11, 2013 at 1:38 AM, bakavic notifications@github.com wrote:

@MiroBarsa https://github.com/MiroBarsa Were you using a Pipeline just
before your get()? I experienced the same problem as you on get(), but I
resolved it by correctly using sync() as recommended in the wiki (
https://github.com/xetorthio/jedis/wiki/AdvancedUsage#pipelining), as
opposed to exec().

Not too sure why exec() for Pipeline exists? I think it's a bit of a
gotcha, seeing that Transactions use exec(), but it is not so for Pipeline.
Another user in the Google Groups also ran into the same trap -
https://groups.google.com/forum/#!topic/jedis_redis/d7CVoCIcCzU


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

@Bam4d
Copy link

Bam4d commented Aug 9, 2016

I am having this issue using the latest jedis. under the same conditions as the original poster above.

my code is using the Optimistic Lock pattern in the jedis docs: http://redis.io/topics/transactions

try (Jedis jedis = jedisPool.getResource()) {

            String permitCountKey = permitCountKey(resourceName);

            List<Object> execResult = null;
            int retryCount = 128;

            while (execResult == null && retryCount-- > 0) {
                log.trace("tryAcquirePermit - {} - attempt: {}", resourceName, retryCount);

                // Only an existing key can be watched, create before calling this method!
                jedis.watch(permitCountKey);

                String permitCountString = jedis.get(permitCountKey);

                // This line will fail under high load as permitCountString sometimes returns "OK"
                int permitCount = (permitCountString == null) ? 0 : Integer.parseInt(permitCountString);

                if (permitCount < resourceLimit) {
                    Transaction transaction = jedis.multi();
                    transaction.incr(permitCountKey);
                    execResult = transaction.exec();
                    jedis.expire(permitCountKey, THREE_HOURS_IN_SECONDS);
                } else {
                    log.trace("tryAcquirePermit - {} - FAILED", resourceName);
                    return false;
                }
            }

            if (retryCount <= 0) {
                // If we couldn't access the permit key, be defensive.
                return false;
            }
        }

Why was this issue closed? Is there a solution to this problem?

@xetorthio
Copy link
Contributor

Make sure the connections sync any pipeline before returning them to the pool.
Most probably this is because your jedis instances has a dirty state.

@Bam4d
Copy link

Bam4d commented Aug 9, 2016

We're not using any pipelines in our code.

There are a few other places where garbage is being returned for some reason.

I see logs like this (all from diffferent parts of the codebase):

java.lang.ClassCastException: java.util.ArrayList cannot be cast to [B
    at redis.clients.jedis.Connection.getStatusCodeReply(Connection.java:196)
    at redis.clients.jedis.Jedis.watch(Jedis.java:1521)
java.lang.ClassCastException: java.util.ArrayList cannot be cast to java.lang.Long
    at redis.clients.jedis.Connection.getIntegerReply(Connection.java:222)
    at redis.clients.jedis.Jedis.zadd(Jedis.java:1333)
    at com.importio.bees.service.TaskService.updateRunTimeoutQueue(TaskService.java:555)

We recently made a change where we try to use the same jedis resource if we have two nested Jedis j = jedis.getResource() calls.

I imagine it might be that the resources like these could be interrupting each other under high load?

it looks to me that the pool connections are getting mixed up and returning the wrong data?

@Bam4d
Copy link

Bam4d commented Aug 9, 2016

Upon further debugging...

Even though we are not using piplined anywhere in our code there are places where
jedis.transaction.pipelinedResponses is not empty, and at this point we are also not in a transaction.

Is this normal?

(Also want to add here that this is a very concurrent system, are there any particular patterns we should follow to avoid issues like this?)

@xetorthio
Copy link
Contributor

The common mistake is to share jedis between threads. This will definitely
throw exceptions like the one you mention.
A Jedis instance is not thread safe. This is why JedisPool exists.

On Tue, Aug 9, 2016 at 12:29 PM Chris Bamford notifications@github.com
wrote:

Upon further debugging...

Even though we are not using piplined anywhere in our code there are
places where
jedis.transaction.pipelinedResponses is not empty, and at this point we
are also not in a transaction.

Is this normal?

(Also want to add here that this is a very concurrent system, are there
any particular patterns we should follow to avoid issues like this?)


You are receiving this because you commented.
Reply to this email directly, view it on GitHub
#397 (comment),
or mute the thread
https://github.com/notifications/unsubscribe-auth/AAIhp6ibYq2IXJ56e-aLeq3sbdxB6OuOks5qeJzQgaJpZM4AcPPn
.

@Bam4d
Copy link

Bam4d commented Aug 10, 2016

Hi, I think i found the issue I was having and it was descibed as you said above..

try(Jedis jedis = jedisPool.getResource()) {
    something.doLambda(() - > runSomthingWithJedisReference(jedis));
} 

The lambda above was using the resource after it was released and probably assigned to another thread.

Removed that and i don't see class cast exceptions anymore, so hopefully OK now..

Thanks for your help :)

Are there any plans to make a thread-safe version of jedis in the future?

@marcosnils
Copy link
Contributor

Are there any plans to make a thread-safe version of jedis in the future?

Probably after migrating to NIO / Netty. We're slowly moving forward on this side but it will probably take some time.

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

4 participants