Redis.close() doesn't close connection pool created in __init__

[ods]

This results in delayed cleanup via Connection.__del__ that causes various errors. Circular references in aioredis make it harder to debug. The problem is especially clearly seen when testing with pytest-asyncio that uses a separate event loop. Just upgrading from aioredis 1.3.1 to 2.0.0 leads to the following errors in setup for random test after the test that creates aioredis client:

Traceback (most recent call last):
  File "/usr/local/lib/python3.8/site-packages/aioredis/connection.py", line 759, in disconnect
    self._writer.close()
  File "/usr/local/lib/python3.8/asyncio/streams.py", line 353, in close
    return self._transport.close()
  File "/usr/local/lib/python3.8/asyncio/selector_events.py", line 690, in close
    self._loop.call_soon(self._call_connection_lost, None)
  File "/usr/local/lib/python3.8/asyncio/base_events.py", line 719, in call_soon
    self._check_closed()
  File "/usr/local/lib/python3.8/asyncio/base_events.py", line 508, in _check_closed
    raise RuntimeError('Event loop is closed')
RuntimeError: Event loop is closed

Here the client is created with one loop and creates transport bound to it, then __del__ is called when some other loop is running causing this transport to close. But this transport uses the loop stored when it's created, which is already closed.

With gc.collect() after each test it becomes much simpler:

Traceback (most recent call last):
  File "/usr/local/lib/python3.8/site-packages/_pytest/runner.py", line 311, in from_call
    result: Optional[TResult] = func()
  File "/usr/local/lib/python3.8/site-packages/_pytest/runner.py", line 255, in <lambda>
    lambda: ihook(item=item, **kwds), when=when, reraise=reraise
  File "/usr/local/lib/python3.8/site-packages/pluggy/hooks.py", line 286, in __call__
    return self._hookexec(self, self.get_hookimpls(), kwargs)
  File "/usr/local/lib/python3.8/site-packages/pluggy/manager.py", line 93, in _hookexec
    return self._inner_hookexec(hook, methods, kwargs)
  File "/usr/local/lib/python3.8/site-packages/pluggy/manager.py", line 84, in <lambda>
    self._inner_hookexec = lambda hook, methods, kwargs: hook.multicall(
  File "/usr/local/lib/python3.8/site-packages/pluggy/callers.py", line 203, in _multicall
    gen.send(outcome)
  File "/usr/local/lib/python3.8/site-packages/_pytest/unraisableexception.py", line 93, in pytest_runtest_teardown
    yield from unraisable_exception_runtest_hook()
  File "/usr/local/lib/python3.8/site-packages/_pytest/unraisableexception.py", line 78, in unraisable_exception_runtest_hook
    warnings.warn(pytest.PytestUnraisableExceptionWarning(msg))
pytest.PytestUnraisableExceptionWarning: Exception ignored in: <socket.socket fd=-1, family=AddressFamily.AF_INET, type=SocketKind.SOCK_STREAM, proto=6>

Traceback (most recent call last):
  File "tests/fixtures.py", line 99, in event_loop
    gc.collect()
ResourceWarning: unclosed <socket.socket fd=34, family=AddressFamily.AF_INET, type=SocketKind.SOCK_STREAM, proto=6, laddr=('172.22.0.5', 51708), raddr=('172.22.0.3', 6379)>

The workaround is simple:

         await client.close()
+        await client.connection_pool.disconnect()

[jonathanspw]

Also ran into this issue. Thanks for the workaround.

Waiting for proper fix upstream.

[Andrew-Chen-Wang]

Please let me know if master branch has resolved this issue.

[agronholm]

Not really. Instead of

RuntimeWarning: coroutine 'Connection.disconnect' was never awaited

we now get

Task was destroyed but it is pending!
task: <Task pending name='Task-2' coro=<Connection.disconnect() done, defined at /home/alex/workspace/asphalt-redis/venv310/lib64/python3.10/site-packages/aioredis/connection.py:794> wait_for=<Future pending cb=[Task.task_wakeup()]>>

I think this change was supposed to be in the 2.0.1 release, but that hasn't been published to the PyPI yet.

[Andrew-Chen-Wang]

Sorry! Thanks for telling me this! 2.0.1 has now been released.

Can you provide code to reproduce?

[agronholm]

I'm currently trying to write a minimal reproducing example that does not involve the Asphalt framework.

[agronholm]

Here:

import asyncio

import aioredis


async def main():
    redis = aioredis.from_url('redis://localhost:63790')
    await redis.set('key', b'value')
    await redis.close()

loop = asyncio.get_event_loop()
loop.run_until_complete(main())
loop.stop()
loop.close()

[agronholm]

Explicitly calling redis.connection_pool.disconnect() after redis.close() fixes the problem. Is there a reason why redis.close() does not do that implicitly?

[Andrew-Chen-Wang]

tl;dr #1256 should remedy this issue of cleanup where redis-py has cleanup in __del__ but we have no mechanism of implicit cleanup.

---

Yes take a look at https://github.com/aio-libs/aioredis-py/blob/56d6b325ee246a3eb0fc8bb6803247c86bb2f494/aioredis/client.py#L863 where a user might want control over a connection pool by passing in their own through Redis's constructor.

The reason this is not done but works fine on redis-py is because all Connection classes do a disconnect on __del__ or garbage collection. Redis client releases conn on its __del__. All of the auto magical cleanups are not available in aioredis because there is no async version of __del__. I will include a PR that can resolve this issue regarding Redis.close() where a new feature will do the automagical close.

[agronholm]

For the record, I think redis-py is also in the wrong in relying on the GC for this, even if that works out for them. Different GC behavior on different Python implementations means that there is no telling when the connections are going to be closed.

[eIGato]

Still reproducible on 2.0.1 and master.

[Andrew-Chen-Wang]

Yes #1256 had not been merged yet. I am migrating this repo to redis-py with the fix since I am unable to merge anything in this github org; I'd expect redis py with asyncio to come out next month at best.

[eIGato]

Cannot reproduce on master. 🎉