redis / redis-py

Redis Python client
MIT License
12.41k stars 2.48k forks source link

redis.asyncio.Sentinel doesn't provide a proper way to clear its child connection pools #3201

Open evgenybf opened 2 months ago

evgenybf commented 2 months ago

Version: What redis-py and what redis version is the issue happening on? redis-py=-5.0.3, master

Platform: What platform / version? (For example Python 3.5.1 on Windows 7 / Ubuntu 15.10 / Azure) python 3.9 Windows 10/ Ubuntu 22.04

Description: Description of your issue, stack traces from errors and code that reproduces the issue

from redis.asyncio.sentinel import Sentinel

# Sentinel.__init__ creates a list of Redis clients - one for each host. Every Redis client contains its own connection pool.
# https://github.com/redis/redis-py/blob/07fc339b4a4088c1ff052527685ebdde43dfc4be/redis/asyncio/sentinel.py#L215
# https://github.com/redis/redis-py/blob/07fc339b4a4088c1ff052527685ebdde43dfc4be/redis/asyncio/client.py#L168C9-L168C65
sentinel = Sentinel([('host1', 26379), ('host2', 26379)], socket_timeout=0.1)

# master_for() creates a new SentinelConnectionPool
# https://github.com/redis/redis-py/blob/07fc339b4a4088c1ff052527685ebdde43dfc4be/redis/asyncio/sentinel.py#L350
master = sentinel.master_for('mymaster', socket_timeout=0.1)

await master.set('foo', 'bar')

# Closes a single Redis ConnectionPool instance
# https://github.com/redis/redis-py/blob/07fc339b4a4088c1ff052527685ebdde43dfc4be/redis/asyncio/client.py#L576
await master.aclose(close_connection_pool=True)

Since, not all connection pools are closed, testing it withIsolatedAsyncioTestCase we get the following warning when sentinel is being garbage collected:

/usr/local/lib/python3.9/site-packages/_pytest/unraisableexception.py:78: PytestUnraisableExceptionWarning: Exception ignored in: <function AbstractConnection.__del__ at 0x772f81b31550>

  Traceback (most recent call last):
    File "/usr/local/lib/python3.9/site-packages/redis/asyncio/connection.py", line 216, in __del__
      self._close()
    File "/usr/local/lib/python3.9/site-packages/redis/asyncio/connection.py", line 223, in _close
      self._writer.close()

A workaround is to close all connections in the sentinel.sentinels list too:

for s in sentinel.sentinels:
    await s.aclose()
WolfEYc commented 1 month ago

+1 the posted workaround works, but it would be nice to just have a

sentinel.aclose()

that closed all sentinels and all connected master + slaves of each sentinel