Unable to set read and write timeouts

[britishtea]

Redic's API currently only provides a connection timeout. The read and write operations have no timeout and no way to provide one.

This is problematic when, for example, the host machine believes it has an active TCP connection to a machine that crashed. The following program illustrates the problem.

Thread.new { TCPServer.new(63790).accept }

r = Redic.new "redis://localhost:63790", 1_000_000
r.call "GET", "foo"

In the worst case the GET foo command will block indefinitely in the above program. Especially when using Redis for caching this is clearly undesired.

The ability to set a write timeout would already be a good improvement. This can be done in a backwards compatible way. I see no reason not to do this.

Setting a read timeout would be trickier, becauseRedic#call is expected to block indefinitely for blocking operations such as BLPOP. Unfortunately the Redic#call interface makes it impossible to provide a read timeout in a backwards compatible way without introducing knowledge of Redis commands into the code base.

I propose the following:

• Redic#call should be changed to use write timeouts and read timeouts. Most Redis commands are not blocking and should be timed out to prevent a program from idling.
• An additional method should be introduced to support blocking commands such as BLPOP. This method will have the exact same interface as Redis#call, but will only set a write timeout (and no read timeout).

Please let me know what you think :)

[britishtea]

@soveran / @cyx Would the ability to set read and write timeouts (regardless of implementation) be a welcome feature in Redic?

[soveran]

@britishtea Sorry I didn't reply before! I agree that it's very tricky, but I also share your opinion that it's a valid concern. I think having a write timeout now is something we should do. As you said, it will be backward compatible. For the read timeout, I really don't have better ideas right now. It makes me sad to have a different method for blocking commands! But I guess in the end we will have to find a solution. Part of the problem is also that I haven't run into that issue, so I didn't feel the pain of the lack of writing timeouts. That would have made the decision easier. What I propose is to implement the timeout for reads right now, see what happens (ie, see if we are breaking something, if somebody complains, etc.), and then think a bit more about the read timeouts. With your proposal, as a separate method, it could be kind of backward compatible, so for me it's the winning approach so far.

[britishtea]

@soveran Thanks for your reply :) This sounds good to me.

I've changed my mind a little bit since submitting the issue. I no longer think it's necessary to introduce a separate method for blocking commands. Introducing a keyword argument to set the read timeout on a per command basis would be enough. It's default value would be something passed to #initialize and can easily be overridden when desired:

r.call "GET", "foo"                # uses a client-wide default read timeout
r.call "BLPOP", "foo", timeout: 60 # overrides the client-wide default for this command only

My reasoning is as follows: when using a blocking command you still want to detect when the other end "left". When using no read timeout at all the detection will happen after the first TCP keep-alive packet is sent. I believe this is 2 hours (!) by default on most machines.

When using a long read timeout of for example 60 seconds, the detection can happen at 60 seconds + the write timeout (assuming BLPOP runs in a loop).

[soveran]

Redic now has a timeout which can be reconfigured at any point, so closing this issue :-)