Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Inconsistency default behaviour on Sync / Async clients against ConnectionError #3194

Open
jonbaine opened this issue Mar 27, 2024 · 1 comment

Comments

@jonbaine
Copy link

jonbaine commented Mar 27, 2024

Version: Redis docker redis:latest. redis-py: 5.0.3

Platform: Ubuntu 22.04.4 LTS

Description:
During a migration from sync to async, we found an important inconsistency on the default behavior between sync/async clients when a ConnectionError happens . This is a really common situation, considering that the redis-ha-proxy is by default configured to close the connections after a while iddle, so this is an absolutely normal situation.

Sync client by default is handling the retry on ConnectionError while the Async client is failing. Probably it's because by default the Redis Retry schema supports those errors -> https://github.com/redis/redis-py/blob/master/redis/retry.py#L14.

This inconsistency happens on all commands, we will explain how to reproduce even in the most simple ping case.

This error impact is that migration from sync to async clients introduces extra complexity by having to deal with inconsistent behavior against network issues.

Reproduce :

First, start a docker container with latest redis image: docker run --name my-redis -p 6379:6379 -d redis

Those are the necessary commands to list / kill connections from clients to redis docker image:

  • List existing connection to redis: ss -tp '( dport = :6379 )'
  • Kill all connections to redis: sudo ss -K -tp '( dport = :6379 )'

Sync client behavior:
Execute standard python command:

Type "help", "copyright", "credits" or "license" for more information.
>>> import redis
>>> redisCli = redis.Redis(
...      host='localhost',
...      port=6379,
...      charset="utf-8",
...      decode_responses=True
...      )
>>> redisCli.ping()
True
>>> print('here we kill connections with the ss command!')
here we kill connections with the ss command!
>>> redisCli.ping()
True
>>> 

Async client behavior:

Execute python with python -m asyncio to be able to await in the python shell.

Type "help", "copyright", "credits" or "license" for more information.
>>> import asyncio
>>> import redis.asyncio as aioredis
>>> redisAsync = aioredis.Redis(host='localhost',
... port = 6379,
... decode_responses=True)
>>> await redisAsync.ping()
True
>>> print('HERE KILL connection with ss command!')
HERE KILL connection with ss command!
>>> await redisAsync.ping()
Traceback (most recent call last):
  File "/home/jonortiz/.pyenv/versions/ais/lib/python3.10/site-packages/redis/asyncio/connection.py", line 473, in send_packed_command
    await self._writer.drain()
  File "/home/jonortiz/.pyenv/versions/3.10.10/lib/python3.10/asyncio/streams.py", line 371, in drain
    await self._protocol._drain_helper()
  File "/home/jonortiz/.pyenv/versions/3.10.10/lib/python3.10/asyncio/streams.py", line 167, in _drain_helper
    raise ConnectionResetError('Connection lost')
ConnectionResetError: Connection lost

The above exception was the direct cause of the following exception:

Traceback (most recent call last):
  File "/home/jonortiz/.pyenv/versions/3.10.10/lib/python3.10/concurrent/futures/_base.py", line 458, in result
    return self.__get_result()
  File "/home/jonortiz/.pyenv/versions/3.10.10/lib/python3.10/concurrent/futures/_base.py", line 403, in __get_result
    raise self._exception
  File "<console>", line 1, in <module>
  File "/home/jonortiz/.pyenv/versions/ais/lib/python3.10/site-packages/redis/asyncio/client.py", line 610, in execute_command
    return await conn.retry.call_with_retry(
  File "/home/jonortiz/.pyenv/versions/ais/lib/python3.10/site-packages/redis/asyncio/retry.py", line 62, in call_with_retry
    await fail(error)
  File "/home/jonortiz/.pyenv/versions/ais/lib/python3.10/site-packages/redis/asyncio/client.py", line 597, in _disconnect_raise
    raise error
  File "/home/jonortiz/.pyenv/versions/ais/lib/python3.10/site-packages/redis/asyncio/retry.py", line 59, in call_with_retry
    return await do()
  File "/home/jonortiz/.pyenv/versions/ais/lib/python3.10/site-packages/redis/asyncio/client.py", line 583, in _send_command_parse_response
    await conn.send_command(*args)
  File "/home/jonortiz/.pyenv/versions/ais/lib/python3.10/site-packages/redis/asyncio/connection.py", line 497, in send_command
    await self.send_packed_command(
  File "/home/jonortiz/.pyenv/versions/ais/lib/python3.10/site-packages/redis/asyncio/connection.py", line 484, in send_packed_command
    raise ConnectionError(
redis.exceptions.ConnectionError: Error UNKNOWN while writing to socket. Connection lost.
>>> 
@alexfandos
Copy link

alexfandos commented Mar 27, 2024

For anyone experiencing this issue, it can be resolved by manually adding the retry parameter to the Redis constructor, as follows:

redisAsync = aioredis.Redis(
    host='localhost',
    port = 6379,
    decode_responses=True,
    retry_on_error=[aioredis.ConnectionError]
)

However, this solution is far from optimal in a migration scenario where we expect consistent behavior across both variants.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

2 participants