Broken connections never unbreak

[Kirk Bulis]

As I look at how the Connection is managed, it seems the connection instance starts life with broken = false, but nowhere in the code does broken get set back to false, say on disconnect().

Why this matters: When trying to connect to a server that currently not reachable, the following code should work:

private val redis: Jedis,
...
if (!redis.isConnected || redis.isBroken) {
  try {
    redis.disconnect()
  }
  catch (eX: Exception) {
  }

  redis.connect()
}
...

do redis.get(), redis.set(), whatever...

but it doesn't because redis.connect() leads to

https://github.com/redis/jedis/blob/253664ff1eac37fdab7ea3e5e738a11f34f8f410/src/main/java/redis/clients/jedis/Connection.java#L237

which will always be true if the connection was ever broken because there is no code anywhere to set broken = false. I think this is a one line fix to simply set broken = false during disconnect() or right before

socket = socketFactory.createSocket();
during connect().

Should I bug it up? and then PR? Am I missing the right way to reset a connection?

Any help much appreciated. Thanks!

[Sazzadul Hoque]

It is expected that the connection would be re-created, not re-connected, when it is broken.

Yeah, there are places for improving docs.

--
You received this message because you are subscribed to the Google Groups "Jedis" group.
To unsubscribe from this group and stop receiving emails from it, send an email to jedis_redis...@googlegroups.com.
To view this discussion on the web visit https://groups.google.com/d/msgid/jedis_redis/4405e1e0-5a15-4fc6-a234-c689d9eae020n%40googlegroups.com.

[Kirk Bulis]

Thanks for replying. Yeah, I suppose creating a new Jedis instance is one way to solve, but I think it also works to reset the "broken" flag when creating a new socket. This seems intuitive and should work:

(server is down)
jedis.connect()
(fails, broken = true)
(sever is now up)
jedis.disconnect()
jedis.connect()

but it won't because of the stickiness of broken = true in the code. So, it seems good to me that the only thing that could be broken is the socket, and we're allocating a new one on connect(), so putting broken = false here:

https://github.com/redis/jedis/blob/253664ff1eac37fdab7ea3e5e738a11f34f8f410/src/main/java/redis/clients/jedis/Connection.java#L226

seems safe.