gresrun / jesque

An implementation of Resque in Java.
http://gresrun.github.io/jesque
Apache License 2.0
630 stars 131 forks source link

Keep alive connection #70

Closed dwilkie closed 9 years ago

dwilkie commented 9 years ago

I'm using the ClientImpl like show below but I'm still getting timeouts:

    final net.greghaines.jesque.client.Client jesqueMtClient = new net.greghaines.jesque.client.ClientImpl(
      jesqueConfig, 15, 15,
      java.util.concurrent.TimeUnit.SECONDS
    );

In ClientImpl on https://github.com/gresrun/jesque/blob/master/src/main/java/net/greghaines/jesque/client/ClientImpl.java#L92 checkConnectionBeforeUse is set to false. However on https://github.com/gresrun/jesque/blob/master/src/main/java/net/greghaines/jesque/client/ClientImpl.java#L155 the method that ensures the jedisConnection is open only runs if checkConnectionBeforeUse is set to true

    private void ensureJedisConnection() {
        if (this.checkConnectionBeforeUse && !JedisUtils.ensureJedisConnection(this.jedis)) {
            authenticateAndSelectDB();
        }
    }

I'm wondering if this is a bug or whether it's intentional. If it's intentional is there another way in which I should be keeping the connection open?

Thanks again for your help.

gresrun commented 9 years ago

Hmmm, it looks like a bug to me. It looks like I introduced it 2.5 years ago in 19989fb !

gresrun commented 9 years ago

My suggestion would be to try changing the keep-alive runner to have the following:

if (!JedisUtils.ensureJedisConnection(jedis)) {
    authenticateAndSelectDB();
}

instead of:

ensureJedisConnection();

If that works, I'll commit a fix.

dwilkie commented 9 years ago

Yep that would be fine but maybe you could simply change the following in the constructor on line https://github.com/gresrun/jesque/blob/master/src/main/java/net/greghaines/jesque/client/ClientImpl.java#L92

  this.checkConnectionBeforeUse = true;

This would ensure that the other constructor works as expected as well

gresrun commented 9 years ago

The whole reason for the second constructor is that the reconnection logic happens on a separate thread and not on the thread performing the client work. by setting this.checkConnectionBeforeUse = false, we prevent the client thread from doing any work in ensureJedisConnection(). The bug was that the reconnection thread wasn't doing any work, either!

dwilkie commented 9 years ago

Ah I see! Then I think your suggestion will work fine.