Warning caused by Redis.__del__()

[Dreamsorcerer]

This method calls create_task(), but then never awaits on that task. This raises a warning in Python, which fails CI for aiohttp-session. This happens even if we've closed the client explicitly.

I was able to fix the warning by patching out the __del__() method: https://github.com/aio-libs/aiohttp-session/pull/616/files#diff-e52e4ddd58b7ef887ab03c04116e676f6280b824ab7469d5d3080e5cba4f2128R21

Explicit is better than implicit, so I'd argue that this method doesn't need to exist, when you already have both a .close() method and support for async with. It's also misleading and a potential performance issue if an async client suddenly starts blocking on IO implicitly. Compare aiohttp's client, which merely raises an error:
https://github.com/aio-libs/aiohttp/blob/master/aiohttp/client.py#L284-L299

[bmoscon]

Also having an issue with this, but the annoying thing on my end is that I've already closed the redis connection myself by explicitly calling (and awaiting) close. When this is called after the loop is stopped/destroyed, it just fails because there is no event loop. Users should be calling close, this should be a best effort thing here to try and close an unclosed connection. Adding a simple check to see the connection is open or not will fix this issue for people who are correctly closing the connection

[seandstewart]

This was definitely a less-than-good idea (for which I take full responsibility) which we should probably update as suggested (change to a ResourceWarning if the connection is unclosed).

Sorry for the pain here - I'll try to see if I've got time to make the change this week, or if anyone wants to make a quick contribution I'll look out for the PR and make sure to give it a timely review.

[bmoscon]

@seandstewart - I made the change but I still get an error:

sys:1: RuntimeWarning: coroutine 'Connection.disconnect' was never awaited
RuntimeWarning: Enable tracemalloc to get the object allocation traceback

(which is less errors than I was originally getting)

Does disconnect need to be called in addition to close? Due to the lack of line number and the fact that there are 15+ instances of disconnect being called in the code, I'm not sure which one is the problem (unless this is something that the user manually needs to call)

[Dreamsorcerer]

Sounds like a generic error caused by calling the disconnect() method and then forgetting to await on it. You'd probably need to share some code that reproduces the issue, but I'm not seeing that one on our tests.

The only other warning left in our tests is an unclosed socket which I can't seem to track down (but, I think it's internal to the Redis client). aio-libs/aiohttp-session#616

[bmoscon]

I enabled tracemalloc and it gave the line in question:

sys:1: RuntimeWarning: coroutine 'Connection.disconnect' was never awaited
Object allocated at (most recent call last):
File "/Users/bryant/source/aioredis-py/aioredis/connection.py", lineno 629
coro = self.disconnect()

I am using a pipeline (as the provided async context manager) but not sure if this is relevant or not

[bmoscon]

yes I removed that code, and see another error. I can open another ticket for that separate issue if you like, but it all stems from the same thing - there isn't really a reliable way in python to have a library auto-clean up by calling coroutines. The end user needs to do that, but this library is littered with instances where it tries to auto clean up instead of calling out the user for not doing so.

[Dreamsorcerer]

Oh wait, my mistake. That's a different class.

So, the change needs to be copied to multiple classes then.

[Andrew-Chen-Wang]

@bmoscon

The end user needs to do that, but this library is littered with instances where it tries to auto clean up instead of calling out the user for not doing so.

I wouldn't mind taking a look at a new ticket with a proposal.

This was definitely a less-than-good idea (for which I take full responsibility) which we should probably update as suggested (change to a ResourceWarning if the connection is unclosed).

Sorry for the pain here - I'll try to see if I've got time to make the change this week, or if anyone wants to make a quick contribution I'll look out for the PR and make sure to give it a timely review.

If anyone would like to make that PR, I'll look out for it (since I can't approve my own).

[PatTheMav]

I was able to fix the warning by patching out the __del__() method: https://github.com/aio-libs/aiohttp-session/pull/616/files#diff-e52e4ddd58b7ef887ab03c04116e676f6280b824ab7469d5d3080e5cba4f2128R21

Thank you for that one, seemed to have fixed my issues as well, though I also explicitly close the redis connection in a context object.