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

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

Open
eoghanmurray opened this issue Oct 27, 2023 · 2 comments
Open

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

eoghanmurray opened this issue Oct 27, 2023 · 2 comments

Comments

@eoghanmurray
Copy link

eoghanmurray commented Oct 27, 2023

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:

  • Am I doing it wrong in terms of creating a new RedisCluster with each request; should I be opening up a pool first? The warning implies that these operations are more expensive than I was thinking
  • Could the await red.aclose() there potentially be expensive and slow down the response slightly?
  • If none of the above, should the __del__ function (presumably triggered by garbage collection) just go ahead and close the connection for me without emitting a warning?

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

@eoghanmurray
Copy link
Author

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
Copy link

rad-pat commented Jan 16, 2024

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?

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

No branches or pull requests

3 participants