Add a "from_pool" argument for the sync client too

Author: kristjanvalur

redis/client.py

@@ -137,7 +137,7 @@ class initializer. In the case of conflicting arguments, querystring
         single_connection_client = kwargs.pop("single_connection_client", False)
         connection_pool = ConnectionPool.from_url(url, **kwargs)
         return cls(
-            connection_pool=connection_pool,
+            from_pool=connection_pool,
             single_connection_client=single_connection_client,
         )
 
@@ -152,6 +152,7 @@ def __init__(
         socket_keepalive=None,
         socket_keepalive_options=None,
         connection_pool=None,
+        from_pool=None,
         unix_socket_path=None,
         encoding="utf-8",
         encoding_errors="strict",
@@ -198,7 +199,7 @@ def __init__(
             if `True`, connection pool is not used. In that case `Redis`
             instance use is not thread safe.
         """
-        if not connection_pool:
+        if not connection_pool and not from_pool:
             if charset is not None:
                 warnings.warn(
                     DeprecationWarning(
@@ -274,8 +275,20 @@ def __init__(
                             "ssl_ocsp_expected_cert": ssl_ocsp_expected_cert,
                         }
                     )
-            connection_pool = ConnectionPool(**kwargs)
-        self.connection_pool = connection_pool
+            from_pool = ConnectionPool(**kwargs)
+        
+        if from_pool is not None:
+            # internal connection pool, expected to be closed by Redis instance
+            if connection_pool is not None:
+                raise ValueError(
+                    "Cannot use both from_pool and connection_pool arguments")
+            self.connection_pool = from_pool
+            self.auto_close_connection_pool = True  # the Redis instance closes the pool
+        else:
+            # external connection pool, expected to be closed by caller
+            self.connection_pool = connection_pool
+            self.auto_close_connection_pool = False  # the user is expected to close the pool
+
         self.connection = None
         if single_connection_client:
             self.connection = self.connection_pool.get_connection("_")
@@ -477,6 +490,9 @@ def close(self):
             self.connection = None
             self.connection_pool.release(conn)
 
+        if self.auto_close_connection_pool:
+            self.connection_pool.disconnect()
+
     def _send_command_parse_response(self, conn, command_name, *args, **options):
         """
         Send a command and parse the response

tests/test_connection.py

@@ -5,9 +5,15 @@
 
 import pytest
 import redis
+from redis import ConnectionPool, Redis
 from redis._parsers import _HiredisParser, _RESP2Parser, _RESP3Parser
 from redis.backoff import NoBackoff
-from redis.connection import Connection, SSLConnection, UnixDomainSocketConnection
+from redis.connection import (
+    Connection,
+    SSLConnection,
+    UnixDomainSocketConnection,
+    parse_url,
+)
 from redis.exceptions import ConnectionError, InvalidResponse, TimeoutError
 from redis.retry import Retry
 from redis.utils import HIREDIS_AVAILABLE
@@ -209,3 +215,84 @@ def test_create_single_connection_client_from_url():
         "redis://localhost:6379/0?", single_connection_client=True
     )
     assert client.connection is not None
+
+
+@pytest.mark.parametrize("from_url", (True, False), ids=("from_url", "from_args"))
+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)
+
+    def get_redis_connection():
+        if from_url:
+            return Redis.from_url(url)
+        return Redis(**url_args)
+
+    r1 = get_redis_connection()
+    assert r1.auto_close_connection_pool is True
+    r1.close()
+
+
+@pytest.mark.parametrize("from_url", (True, False), ids=("from_url", "from_args"))
+def test_redis_connection_pool(request, from_url):
+    """Verify that basic Redis instances using `connection_pool`
+    have auto_close_connection_pool set to False"""
+
+    url: str = request.config.getoption("--redis-url")
+    url_args = parse_url(url)
+
+    pool = None
+
+    def get_redis_connection():
+        nonlocal pool
+        if from_url:
+            pool = ConnectionPool.from_url(url)
+        else:
+            pool = ConnectionPool(**url_args)
+        return Redis(connection_pool=pool)
+
+    called = 0
+
+    def mock_disconnect(_):
+        nonlocal called
+        called += 1
+
+    with patch.object(ConnectionPool, "disconnect", mock_disconnect):
+        with get_redis_connection() as r1:
+            assert r1.auto_close_connection_pool is False
+
+    assert called == 0
+    pool.disconnect()
+
+
+@pytest.mark.parametrize("from_url", (True, False), ids=("from_url", "from_args"))
+def test_redis_from_pool(request, from_url):
+    """Verify that basic Redis instances using `from_pool`
+    have auto_close_connection_pool set to True"""
+
+    url: str = request.config.getoption("--redis-url")
+    url_args = parse_url(url)
+
+    pool = None
+
+    def get_redis_connection():
+        nonlocal pool
+        if from_url:
+            pool = ConnectionPool.from_url(url)
+        else:
+            pool = ConnectionPool(**url_args)
+        return Redis(from_pool=pool)
+
+    called = 0
+
+    def mock_disconnect(_):
+        nonlocal called
+        called += 1
+
+    with patch.object(ConnectionPool, "disconnect", mock_disconnect):
+        with get_redis_connection() as r1:
+            assert r1.auto_close_connection_pool is True
+
+    assert called == 1
+    pool.disconnect()