redis / redis-py

Redis Python client
MIT License
12.65k stars 2.52k forks source link

Why warn on e.g. "Unclosed RedisCluster client"? #3026

Open eoghanmurray opened 1 year ago

eoghanmurray commented 1 year ago

I've come to the library from aredis and running my same code, a web application built on aiohttp. I'm wondering why this library warns if a redis connection has not been closed in the __del__ function, rather than just closing it?

Unclosed RedisCluster client
client: <redis.asyncio.cluster.RedisCluster object at 0x7f86d1c27280>

Although it now is obvious, It took me a while to track down what exactly was triggering the warning; I previously didn't know that connections were expected to await aclose

from aiohttp import web
from redis.asyncio.cluster import RedisCluster

async def endpoint(request=False, source=''):
    red = RedisCluster(
        REDIS_CX[0], REDIS_CX[1], password=REDIS_PWD,
         decode_responses=decode_responses,
    )
    ...
    await red.aclose()    #   <----------------FIX
    return web.json_response(ret)

if __name__ == '__main__':
    app = web.Application()
    app.add_routes([
        web.get('/endpoint', endpoint),
    ])
    web.run_app(app, host=ip_host, port=port, ssl_context=ssl_context)

So my questions are:

I'd be happy to update docs etc. if there are changes needed.

eoghanmurray commented 11 months ago

https://redis.readthedocs.io/en/stable/examples/asyncio_examples.html#Connecting-and-Disconnecting "Utilizing asyncio Redis requires an explicit disconnect of the connection since there is no asyncio deconstructor magic method."

I'm wondering if the above line makes sense if it's true that the __del__ function could serve the purpose as a deconstructor?

rad-pat commented 9 months ago

I think to fix your problem, you can just use the async context manager, it calls the aclose

async def endpoint(request=False, source=''):
    async with RedisCluster(
        REDIS_CX[0], REDIS_CX[1], password=REDIS_PWD,
         decode_responses=decode_responses,
    ) as red:
        red.do_stuff()
        return web.json_response(ret)

Creating a connection every request seems a little heavy handed in my opinion, perhaps opening a connection and storing the reference on web server and then call aclose on server shutdown might be better?