AsyncRedisManager: reconnect path orphans the previous Redis client without awaiting close

[zxzinn]

AsyncRedisManager._redis_connect overwrites self.redis and self.pubsub without closing the previous ones. The old Redis client is never awaited closed, so its TCP socket (and TLS session for rediss://) is leaked.

redis.asyncio.Redis requires await aclose() for cleanup; its __del__ only emits ResourceWarning: Unclosed client session because async cleanup cannot run in __del__. The manager owns the client, so the manager must close it.

_redis_connect runs on every reconnect:

• _publish calls it after a failed publish
• _redis_listen_with_retries calls it after any RedisError

Under sustained Redis pressure these fire repeatedly, leaking one socket per reconnect.

Repro: https://github.com/zxzinn/python-socketio-redis-leak-repro

warmup        sockets=1
reconnect #1  sockets=2
reconnect #2  sockets=3
reconnect #3  sockets=4
reconnect #4  sockets=5
reconnect #5  sockets=6

Each iteration emits ResourceWarning: unclosed Connection.

Fix: await self.pubsub.aclose() and await self.redis.aclose() before reassigning. _redis_connect is sync today but both callers are already async.

Reproduces on 5.16.1 and 5.8.0.

[miguelgrinberg]

The failures that come out of this module are disconnections. So there is nothing to close, and in fact a call to close will fail because the socket is already gone.

[zxzinn]

i don't think the socket is necessarily gone at that point. the failure path is one operation raising RedisError, but self.redis is still the live client object with a pool that may have other working connections. then _redis_connect overwrites it:

def _redis_connect(self):
    self.redis = aioredis.Redis.from_url(self.redis_url, ...)
    self.pubsub = self.redis.pubsub(ignore_subscribe_messages=True)

redis-py treats not calling aclose() as a bug:

https://github.com/redis/redis-py/blob/653d9ef00fc687e72c0bb065d2c61c664b13b63b/redis/asyncio/client.py#L574-L586

ran 5 forced reconnects against a local redis, kept only the latest client like the manager does. CLIENT LIST on the server:

baseline server clients: 3
after _redis_connect+publish #1: server clients = 4
after _redis_connect+publish #2: server clients = 5
after _redis_connect+publish #3: server clients = 6
after _redis_connect+publish #4: server clients = 7
after _redis_connect+publish #5: server clients = 8

the 5 abandoned cmd=publish connections were still ESTABLISHED on the server.

aclose() after the socket is killed from the server side also doesn't raise on redis-py 5.3, it just no-ops:

https://github.com/redis/redis-py/blob/653d9ef00fc687e72c0bb065d2c61c664b13b63b/redis/asyncio/client.py#L574-L613

repro: https://github.com/zxzinn/python-socketio-redis-leak-repro

[miguelgrinberg]

This code was introduced in Feb 2017. At the time there were no re-connection support. The re-connection feature you are looking at was added in Jan 2018. Since then, I have received no issues that include the resource warning that you reference.

Have you seen this happen during actual use of this package? Because of the way you write, I think you haven't, you are just doing static analysis on the code, likely with an LLM. If you want to convince me that this is worth fixing, you need to show a reproduction with actual running code, without invoking private methods that trigger abnormal situations. Create a manager instance, connect it to Redis, make it raise a legitimate RedisError to force a reconnection, then show that there is a leaked socket.

[zxzinn]

I've reproduced using a standalone api server without touching the private methods, and I do use the package, but it was difficult to reproduce for me and hard to express, it was a too specific problem happens in gunicorn servers with uvicorn workers and gc interactions, and I did observed the leaked pattern in production using gc traces, if you still have time to take a look, the repo is right here:
https://github.com/zxzinn/python-socketio-redis-leak-repro

[miguelgrinberg]

Yeah, okay, injecting garbage in the Redis socket is not what I would call "legitimate", and less so when you continue to resort to LLM generated code to try to make your point.

This is a low priority issue in my view.