Retrying on timeouts not working when executing Redis Pipeline

[pall-j]

Version: 4.5.5

Platform: Python 3.10.10, Ubuntu 22.04

Description:

When the Redis client is initialized with retry_on_timeout set to True or retry_on_error set to [redis.exceptions.TimeoutError, socket.timeout] and connection_pool is not provided, the timeout retrying does not work in client's pipeline execution.

For example:

my_redis_client = redis.StrictRedis(
    host='...',
    port=6380,
    password='...',
    ssl=True,
    socket_timeout=30,
    socket_connect_timeout=30,
    retry=retry,
    retry_on_timeout=True,
)
pipe = my_redis_client.pipeline()
pipe.incr(name='key')
pipe.execute()  # Retrying on timeout won't work here

Problem found:

Inside Redis init method only retry_on_error is propagated through newly created kwargs into the created ConnectionPool (line 1043 in redis/client.py) with retry_on_timeout causing TimeoutError to be added to retry_on_error (line 988 in redis/client.py).

Subsequently, when a Redis Pipeline is initialized from the client using the pipeline method (1096 in redis/client.py), it is initialized using the ConnectionPool created in the Redis constructor.

When we execute the pipeline using the execute method (line 2100 in redis/client.py), conn.retry.call_with_retry will be called, where conn is a Connection object initiated from the ConnectionPool with retry_on_error set, and retry_on_timeout not set as it was not propagated into ConnectionPool and thus to Connection.

When a timeout occurs while executing the pipeline, _disconnect_raise_reset(conn, error) method of the Redis Pipeline is called (line 2080 in redis/client.py). The problem with this method is that it will always re-raise the TimeoutError due to conn.retry_on_timeoutalways being False. The reason is that conn was created from the ConnectionPool, which was initialized without retry_on_timeout being set (in the Redis constructor).

Possible fix:

Use retry_on_error instead of retry_on_timeout inside _disconnect_raise_reset.

[mburcher]

I note that this bug also affects instances of PubSub. The retry_on_timeout parameter from Redis is not propagated to the ConnectionPool and it therefore not propagated to the Connection.

Example for demonstration. For redis-py<=4.0.2 this listens "forever".
For redis-py==4.5.5 TimeoutError is raised.

>>> import redis
>>> redis_cli = redis.Redis(socket_timeout=10, retry_on_timeout=True)
>>> pubsub = redis_cli.pubsub()
>>> pubsub.subscribe("channel")
>>> for item in pubsub.listen():
...     print(item["type"])
... 
subscribe
Traceback (most recent call last):
  File "/venv_test/lib/python3.7/site-packages/redis/connection.py", line 210, in _read_from_socket
    data = self._sock.recv(socket_read_size)
socket.timeout: timed out

During handling of the above exception, another exception occurred:

Traceback (most recent call last):
  File "<stdin>", line 1, in <module>
  File "/venv_test/lib/python3.7/site-packages/redis/client.py", line 1664, in listen
    response = self.handle_message(self.parse_response(block=True))
  File "/venv_test/lib/python3.7/site-packages/redis/client.py", line 1542, in parse_response
    response = self._execute(conn, try_read)
  File "/venv_test/lib/python3.7/site-packages/redis/client.py", line 1520, in _execute
    lambda error: self._disconnect_raise_connect(conn, error),
  File "/venv_test/lib/python3.7/site-packages/redis/retry.py", line 49, in call_with_retry
    fail(error)
  File "/venv_test/lib/python3.7/site-packages/redis/client.py", line 1520, in <lambda>
    lambda error: self._disconnect_raise_connect(conn, error),
  File "/venv_test/lib/python3.7/site-packages/redis/client.py", line 1507, in _disconnect_raise_connect
    raise error
  File "/venv_test/lib/python3.7/site-packages/redis/retry.py", line 46, in call_with_retry
    return do()
  File "/venv_test/lib/python3.7/site-packages/redis/client.py", line 1519, in <lambda>
    lambda: command(*args, **kwargs),
  File "/venv_test/lib/python3.7/site-packages/redis/client.py", line 1540, in try_read
    return conn.read_response(disconnect_on_error=False)
  File "/venv_test/lib/python3.7/site-packages/redis/connection.py", line 874, in read_response
    response = self._parser.read_response(disable_decoding=disable_decoding)
  File "/venv_test/lib/python3.7/site-packages/redis/connection.py", line 347, in read_response
    result = self._read_response(disable_decoding=disable_decoding)
  File "/venv_test/lib/python3.7/site-packages/redis/connection.py", line 357, in _read_response
    raw = self._buffer.readline()
  File "/venv_test/lib/python3.7/site-packages/redis/connection.py", line 260, in readline
    self._read_from_socket()
  File "/venv_test/lib/python3.7/site-packages/redis/connection.py", line 223, in _read_from_socket
    raise TimeoutError("Timeout reading from socket")
redis.exceptions.TimeoutError: Timeout reading from socket

Possible fixes:

1. Extend approach in Fix timeout retrying on pipeline execution #2812 . Apply equivalent changes to PubSub._disconnect_raise_connect
2. Pass the retry_on_timeout parameter to the kwargs used to construct the ConnectionPool. (This is the approach taken in asyncio/client.py.)