Repository: redis/redis-py
Pull request #2859 (2023-07-19 16:55:07)

Fix socket garbage collection
### Pull Request check-list

_Please make sure to review and check all of these items:_

- [ ] Does `$ tox` pass with this change (including linting)?
- [ ] Do the CI tests pass with this change (enable it first in your forked repo and wait for the github action build to finish)?
- [ ] Is the new or changed code fully tested?
- [ ] Is a documentation update included (if this change modifies existing APIs, or introduces new ones)?
- [ ] Is there an example added to the examples folder (if applicable)?
- [x] Was the change added to CHANGES file?

_NOTE: these things are not required to open a PR and can be done
afterwards / while the PR is open._

### Description of change

Issue #2831 shows how connections are kept open when they are simply discarded.
The problem is two-fold:

1. Redis connections opened with `from-url` don't have their `auto_close_connection` boolean set to `True`, as is the default for a pool which is created automatically.  Therefore, when `Redis.close()` was called, the pool would not close its collections.
2. Even so, garbage collection of the `Connection` objects _ought_ to have closed the sockets associated with the underlying `StreamWriter` objects.  This apparently does not reliably happen, so we re-instate a `__del__` handler for the `AbstractConnection` which does the bare minimum required, calling `StreamWriter.close()`.
3. Add an `auto_close_connection_pool` argument to `asyncio.Redis.from_url()` which defaults to true, same as for `asyncio.Redis()`

A fix was added to an unrelated test which caused unit tests to fail on non-linux platforms.

###  Update

I've changed the PR so as to remove the _2_, above.  In general for `asyncio` Python, socket cleanup is _not_ done on garbage collection (since it involves async operations).  Instead, it is assumed that a well written program performs proper disconnects as needed.  `__del__` handlers instead are used to emit _warnings_, that is, if a socket (or other resource) is found to be not closed when purging an object, a warning is emitted.  This is to make sure resources are released properly by the application.

 Fixing of (1) above ensures that the connection pool is indeed cleaned up correctly.  Trying to put cleanup and IO logic into destructors is in general not thought to be a good practice and should be avoided whenever possible.  Correct program operation should not rely on the prompt execution of `__del__` handlers, and implementing them would deprive us of the built-in resource warnings which would otherwise be emitted.

kristjanvalur: @dvora-h you may want to see the latest change.  Arguments intended for the "Redis" class are now explicit in `from_url`, while the `kwargs` get passed to the `ConnectionPool` constructor.  This makes the docstring more correct.

Patch:
diff --git a/CHANGES b/CHANGES
index 49f87cd3..363f2b92 100644
--- a/CHANGES
+++ b/CHANGES
@@ -1,3 +1,4 @@
+    * Fix #2831, add auto_close_connection_pool=True arg to asyncio.Redis.from_url()
     * Fix incorrect redis.asyncio.Cluster type hint for `retry_on_error`
     * Fix dead weakref in sentinel connection causing ReferenceError (#2767)
     * Fix #2768, Fix KeyError: 'first-entry' in parse_xinfo_stream.
diff --git a/redis/asyncio/client.py b/redis/asyncio/client.py
index 111df241..31e27a44 100644
--- a/redis/asyncio/client.py
+++ b/redis/asyncio/client.py
@@ -104,7 +104,13 @@ class Redis(
     response_callbacks: MutableMapping[Union[str, bytes], ResponseCallbackT]
 
     @classmethod
-    def from_url(cls, url: str, **kwargs):
+    def from_url(
+        cls,
+        url: str,
+        single_connection_client: bool = False,
+        auto_close_connection_pool: bool = True,
+        **kwargs,
+    ):
         """
         Return a Redis client object configured from the given URL
 
@@ -144,12 +150,13 @@ class Redis(
         arguments always win.
 
         """
-        single_connection_client = kwargs.pop("single_connection_client", False)
         connection_pool = ConnectionPool.from_url(url, **kwargs)
-        return cls(
+        redis = cls(
             connection_pool=connection_pool,
             single_connection_client=single_connection_client,
         )
+        redis.auto_close_connection_pool = auto_close_connection_pool
+        return redis
 
     def __init__(
         self,


Test patch:
diff --git a/tests/test_asyncio/test_connection.py b/tests/test_asyncio/test_connection.py
index 09960fd7..9a729392 100644
--- a/tests/test_asyncio/test_connection.py
+++ b/tests/test_asyncio/test_connection.py
@@ -12,7 +12,7 @@ from redis._parsers import (
     _AsyncRESPBase,
 )
 from redis.asyncio import Redis
-from redis.asyncio.connection import Connection, UnixDomainSocketConnection
+from redis.asyncio.connection import Connection, UnixDomainSocketConnection, parse_url
 from redis.asyncio.retry import Retry
 from redis.backoff import NoBackoff
 from redis.exceptions import ConnectionError, InvalidResponse, TimeoutError
@@ -278,3 +278,39 @@ async def test_connection_disconect_race(parser_class):
 def test_create_single_connection_client_from_url():
     client = Redis.from_url("redis://localhost:6379/0?", single_connection_client=True)
     assert client.single_connection_client is True
+
+
+@pytest.mark.parametrize("from_url", (True, False))
+async def test_pool_auto_close(request, from_url):
+    """Verify that basic Redis instances have auto_close_connection_pool set to True"""
+
+    url: str = request.config.getoption("--redis-url")
+    url_args = parse_url(url)
+
+    async def get_redis_connection():
+        if from_url:
+            return Redis.from_url(url)
+        return Redis(**url_args)
+
+    r1 = await get_redis_connection()
+    assert r1.auto_close_connection_pool is True
+    await r1.close()
+
+
+@pytest.mark.parametrize("from_url", (True, False))
+async def test_pool_auto_close_disable(request, from_url):
+    """Verify that auto_close_connection_pool can be disabled"""
+
+    url: str = request.config.getoption("--redis-url")
+    url_args = parse_url(url)
+
+    async def get_redis_connection():
+        if from_url:
+            return Redis.from_url(url, auto_close_connection_pool=False)
+        url_args["auto_close_connection_pool"] = False
+        return Redis(**url_args)
+
+    r1 = await get_redis_connection()
+    assert r1.auto_close_connection_pool is False
+    await r1.connection_pool.disconnect()
+    await r1.close()
diff --git a/tests/test_connect.py b/tests/test_connect.py
index b233c67e..f07750dc 100644
--- a/tests/test_connect.py
+++ b/tests/test_connect.py
@@ -61,6 +61,8 @@ def test_tcp_ssl_connect(tcp_address):
 
 def _assert_connect(conn, server_address, certfile=None, keyfile=None):
     if isinstance(server_address, str):
+        if not _RedisUDSServer:
+            pytest.skip("Unix domain sockets are not supported on this platform")
         server = _RedisUDSServer(server_address, _RedisRequestHandler)
     else:
         server = _RedisTCPServer(
@@ -113,24 +115,29 @@ class _RedisTCPServer(socketserver.TCPServer):
         return connstream, fromaddr
 
 
-class _RedisUDSServer(socketserver.UnixStreamServer):
-    def __init__(self, *args, **kw) -> None:
-        self._ready_event = threading.Event()
-        self._stop_requested = False
-        super().__init__(*args, **kw)
+if hasattr(socket, "UnixStreamServer"):
 
-    def service_actions(self):
-        self._ready_event.set()
+    class _RedisUDSServer(socketserver.UnixStreamServer):
+        def __init__(self, *args, **kw) -> None:
+            self._ready_event = threading.Event()
+            self._stop_requested = False
+            super().__init__(*args, **kw)
 
-    def wait_online(self):
-        self._ready_event.wait()
+        def service_actions(self):
+            self._ready_event.set()
 
-    def stop(self):
-        self._stop_requested = True
-        self.shutdown()
+        def wait_online(self):
+            self._ready_event.wait()
 
-    def is_serving(self):
-        return not self._stop_requested
+        def stop(self):
+            self._stop_requested = True
+            self.shutdown()
+
+        def is_serving(self):
+            return not self._stop_requested
+
+else:
+    _RedisUDSServer = None
 
 
 class _RedisRequestHandler(socketserver.StreamRequestHandler):