Need clarification and better documentation on socket_timeout, socket_connect_timeout, and blocking commands

[nickwilliams-eventbrite]

Version: 2.10.6 but thinking about upgrading to 3.3.11 to hopefully address this and need clarification

Platform: Ubuntu 16.04 on Python 3.7

Description: We've having an issue that is making us consider upgrading our stack to 3.3.11, but we need some clarification on some things that are insufficiently documented and that I've been unable to determine from reading through the code.

The issue at hand is that, when Sentinel fails over due to a hard failed master, the clients will block for many minutes hoping to receive some response from the now-dead master, and then the Sentinel client doesn't appear to load new master info as far as I can tell. We think what we need to employ here is some socket timeouts (socket_connect_timeout and/or socket_timeout) and possibly also the new health_check_interval parameter added to 3.3.x.

But I have some concerns around these issues because the documentation doesn't really explain what the three arguments in question do. So here are my two questions that I'm hoping to get quick answers for and also hoping that the documentation can be updated to reflect.

• Is health_check_interval only useful when socket_timeout is also set? Or socket_connect_timeout? Or can it be useful with either and more useful with both? Basically, how do these three options interact?
• I get that socket_connect_timeout is about timing out the connection-establishment process and socket_timeout is about send and receive, but what isn't clear to me is how socket_timeout deals with blocking Redis commands like BLPOP. When we call commands like LLEN or RPUSH, we would like those to time out very quickly (say, ~2-3 seconds). However, we also make BLPOP calls, and for those we use anything from timeout=2 to timeout=120 (depending on the use case) to wait for data to be available on the list. So, given that, if we have socket_timeout set to 5 seconds and we call BLPOP with timeout=120, will it timeout in 5 seconds (bad) or 120 seconds (intended)? If 5 seconds, can the blocking commands be modified so that their timeouts override socket_timeout if higher than socket_timeout?

[andymccurdy]

health_check_interval can be useful even when socket_timeout is not set. When health checks are enabled by setting health_check_interval, a connection that has been idle for >= health_check_interval seconds will attempt a roundtrip ping/pong check to the Redis server just before sending a command. If the ping/pong fails, the socket connection is closed and re-established. Assuming the post-reconnection ping/pong is successful, the actual command is then executed and its response returned. The goal of health checks is to make it safe for redis-py to handle various types of connection errors without the user needing to try/except around every call to Redis in their code.

socket_timeout can be used in conjunction with this. If you're running in an environment where TCP connections can become stalled (often due to various networking hardware between the client and server), socket_timeout will prevent any command, including the health check ping/pong, from waiting more than socket_timeout seconds.

socket_timeout knows nothing about Redis commands that are blocking like BLPOP. The timeout argument to BLPOP (and a handful of other commands) instructs the Redis server to return a null reply after timeout seconds if the list(s) are still empty. If you're using socket_timeout, then you should be specifying a timeout to BLPOP that is less than the socket_timeout value. The strategy below works well:

r = redis.Redis(..., socket_timeout=5)

while True:
    response = r.blpop('my-list', timeout=3)
    if response is not None:
        break
# do stuff with response here...

[nickwilliams-eventbrite]

All of that information is very helpful. I think you could just copy and paste most of that into the documentation with a bit of cleanup and it would satisfy the underlying issue of this bug report (missing documentation).

I would like to explore the socket_timeout relationship with blocking commands a bit more, though. I spent some more time this morning reading through some of the code in redis-py and had already came to the same conclusion myself, but I wonder if this is something we can improve. It would require adding a timeout kwarg to several methods in the stack between the command method (e.g., blpop) and the socket (which AFAICT is Client.execute_command [already has **options we can use], Client.parse_response [already has **options we can use], Connection.read_response, PythonParser.read_response, and finally SocketBuffer.readline [SocketBuffer._read_from_socket already has a custom_timeout argument]).

I'm thinking along the lines of this: Blocking commands that accept a timeout currently merge that timeout into the arguments sent with the command. The change I'm suggesting is that these commands also send that timeout to execute_command. It pops the timeout (if it exists) off its **options (so as not to pass them elsewhere) and sends it to parse_response, which pops it of its **options and sends it to read_response and then read_response and then readline and then _read_from_socket. Then, at this point, _read_from_socket already has logic in place to temporarily override and then restore the timeout.

This is a simplification, it would be a bit more complicated than this, because there are two different parsers, etc., but I think you get the gist of what I'm suggesting.

Are you amenable to such an improvement? If so, would you prefer to implement it yourself, or is a pull request the quickest way to knock out something like this?

[andymccurdy]

If such an improvement was made, we would need to pad the override to socket_timeout slightly. For example, r.blpop('my-list', timeout=5) would want to send timeout=5 to Redis but would want to override socket_timeout to something > 5. If both were set to 5, the client's overridden socket_timeout would in general be triggered first and you might lose a valid value from the server that came in at the 4.9 second mark.

And I could see a desire for that padding to be controllable by users. There are some edge cases where folks use redis-py in a client application to talk to a remote Redis server over the internet. In such a case you'd probably want a multi-second padding on socket_timeout to account for random internet latency. For the more typical use case of having a Redis server in close proximity to the client, you might want to only pad 1 second (or perhaps a fraction of a second) to the timeout.

I do agree with you that it would be nice for developers to be able to reliably use BLPOP with a timeout without having to know or care about how the socket_timeout is configured in various environments outside of their control.

Perhaps an easier way (both for redis-py and for users to reason about) might be to make the while loop strategy above more convenient. We could build a helper that encapsulated the while loop and specified a timeout value that was somehow based on the client's socket_timeout configuration. Such a helper could accept a jitter value (I'm not sold on the name) with a reasonable default (say 1 for this example). It would then compute the timeout value to pass to Redis using socket_timeout - jitter.

r = redis.Redis(..., socket_timeout=5)

def helper(client, command_func, keys, jitter=1):
    command_timeout = None
    socket_timeout = client.connection_pool.connection_kwargs.get('socket_timeout', None)
    if socket_timeout is not None:
        command_timeout = int(socket_timeout - jitter)
        # avoid negative timeouts. also avoid timeout = 0 as that will eat CPU time 
        if command_timeout <= 0:
            command_timeout = 1  #??? or maybe 0.5?
    while True:
        response = command_func(keys, timeout=command_timeout)
        if response is not None:
            return response

# and use the helper func
item = helper(r, r.blpop, ['my-keys'])

After writing this, I'm left uncertain whether this is too magical or not.

[github-actions]

This issue is marked stale. It will be closed in 30 days if it is not updated.