Repository: redis/redis-py

Issue:
`auto_close_connection_pool` ignored
This defect pertains to issues discovered with #2831 

In async mode, it is important that a `Redis` object which holds its own pool, can be told to close it when the `Redis` object
is closed, to free the user from such hassles.  Examples in the code include
``` python
from redis.asyncio.client import Redis
client = Redis(connection_pool=BlockingConnectionPool())
```
when this client is closed, the internal connection pool remains open, i.e. its connections are not shut down.

The `Redis` class includes a `auto_close_connection_pool` argument.  However, this is _ignored_ when a connection pool
is provided to the constructor.  Presumably the thinking is that the caller will manage that pool himself.
But that makes the above kind of pattern, also used within the code, unusable.

```python
client = Redis(connection_pool=BlockingConnectionPool(), auto_close_connection_pool=True)
assert client.auto_close_connection_pool=False
```

Instead, it is necessary to have code like this, which is very poor practice:
```python
client = Redis(connection_pool=BlockingConnectionPool(), auto_close_connection_pool=True)
client.auto_close_connection_pool=True
```

I propose, that we either:

- Always honour the `auto_close_connection_pool` argument.  
- add a `own_connection_pool:ConnectionPool` argument, to be used instead of `connection_pool`.
- rename `auto_close_connection_pool` to `own_pool`, or something less cumbersome.
- add a `construct_connection_pool` argument which takes a callable which returns a connection pool, one which Redis considers its own.

At any rate, it should be easy to create a connection pool, and hand it to the `Redis` constructor for it to "own" without
having to fudge an internal property afterwards.

In addition, I propose that the same mechanics be added to the non-`async` parts of the library.  Currently the `redis.Redis` objects do not close connection pools, instead the library relies on the prompt execution of `__del__()` methods, or explicit `ConnectionPool.disconnect()` calls.  
Relying on garbage collection to free resources is considered bad programming practice in Python, where resource management should in general be more explicit.  It is particularly frowned upon for async code, since executing asynchronous code there is not a good idea(*).  the synchronous code should receive whatever solution we choose for the above as well, so that pools owned by a `Redis` client object are also disconnected when those Redis objects are closed.

> (*)  `__del__()` methods are not async.  While it is possible to use something like `asyncio.run` within such a function, it will __block__ the rest of the program.  Also, `__del__()` methods are often invoked during program cleanup when it is simply not possible to do anything and trying to do so will produce errors and confusion.

### Update

In pr #2913 I have updated a `Redis.from_pool()` method to use when handing over a `ConnectionPool` to be _owned_ by the `Redis` instance.  I think this is a much nicer way than to heap on more constructor arguments to `Redis`

Discussion:
codecov-commenter: ## [Codecov](https://app.codecov.io/gh/redis/redis-py/pull/2913?src=pr&el=h1&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=redis) Report
Patch coverage: **`97.93%`** and project coverage change: **`-13.61%`** :warning:
> Comparison is base [(`19b55c6`)](https://app.codecov.io/gh/redis/redis-py/commit/19b55c62389c890a96dd611e28aaaedba7506720?el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=redis) 91.27% compared to head [(`d59e207`)](https://app.codecov.io/gh/redis/redis-py/pull/2913?src=pr&el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=redis) 77.67%.

:exclamation: Your organization is not using the GitHub App Integration. As a result you may experience degraded service beginning May 15th. Please [install the Github App Integration](https://github.com/apps/codecov) for your organization. [Read more](https://about.codecov.io/blog/codecov-is-updating-its-github-integration/?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=redis).

<details><summary>Additional details and impacted files</summary>


```diff
@@             Coverage Diff             @@
##           master    #2913       +/-   ##
===========================================
- Coverage   91.27%   77.67%   -13.61%     
===========================================
  Files         126      126               
  Lines       32391    32513      +122     
===========================================
- Hits        29566    25255     -4311     
- Misses       2825     7258     +4433     
```


| [Files Changed](https://app.codecov.io/gh/redis/redis-py/pull/2913?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=redis) | Coverage Δ | |
|---|---|---|
| [redis/asyncio/connection.py](https://app.codecov.io/gh/redis/redis-py/pull/2913?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=redis#diff-cmVkaXMvYXN5bmNpby9jb25uZWN0aW9uLnB5) | `82.37% <ø> (-0.61%)` | :arrow_down: |
| [tests/test\_asyncio/test\_connection.py](https://app.codecov.io/gh/redis/redis-py/pull/2913?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=redis#diff-dGVzdHMvdGVzdF9hc3luY2lvL3Rlc3RfY29ubmVjdGlvbi5weQ==) | `93.58% <96.29%> (+0.57%)` | :arrow_up: |
| [tests/test\_connection.py](https://app.codecov.io/gh/redis/redis-py/pull/2913?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=redis#diff-dGVzdHMvdGVzdF9jb25uZWN0aW9uLnB5) | `98.27% <97.95%> (-0.14%)` | :arrow_down: |
| [redis/asyncio/client.py](https://app.codecov.io/gh/redis/redis-py/pull/2913?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=redis#diff-cmVkaXMvYXN5bmNpby9jbGllbnQucHk=) | `92.07% <100.00%> (+0.07%)` | :arrow_up: |
| [redis/asyncio/sentinel.py](https://app.codecov.io/gh/redis/redis-py/pull/2913?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=redis#diff-cmVkaXMvYXN5bmNpby9zZW50aW5lbC5weQ==) | `86.90% <100.00%> (-0.46%)` | :arrow_down: |
| [redis/client.py](https://app.codecov.io/gh/redis/redis-py/pull/2913?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=redis#diff-cmVkaXMvY2xpZW50LnB5) | `91.79% <100.00%> (-0.02%)` | :arrow_down: |
| [redis/sentinel.py](https://app.codecov.io/gh/redis/redis-py/pull/2913?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=redis#diff-cmVkaXMvc2VudGluZWwucHk=) | `86.95% <100.00%> (ø)` | |
| [tests/test\_sentinel.py](https://app.codecov.io/gh/redis/redis-py/pull/2913?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=redis#diff-dGVzdHMvdGVzdF9zZW50aW5lbC5weQ==) | `99.40% <100.00%> (+0.05%)` | :arrow_up: |

... and [26 files with indirect coverage changes](https://app.codecov.io/gh/redis/redis-py/pull/2913/indirect-changes?src=pr&el=tree-more&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=redis)


</details>

[:umbrella: View full report in Codecov by Sentry](https://app.codecov.io/gh/redis/redis-py/pull/2913?src=pr&el=continue&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=redis).   
:loudspeaker: Have feedback on the report? [Share it here](https://about.codecov.io/codecov-pr-comment-feedback/?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=redis).

chayim: Here, my concern is primarly around breaking the connection interface. I realize that async is in flux, and frankly many things will be - but this interface is public and exposed, and not internal. Maybe there's another way - without breaking the user contract?
kristjanvalur: I understand.  I think we can leave this attribute in place.  I'm sure you agree that it feels cludgy, and long.  
Do we have a standard schedule and mechanism to deprecate functionality / arguments in redis-py?
chayim: I definitely agree is kludgey... 

I generally dislike these interface (sync and asycn.. heck even cluster). But we should break accordingly in the future.

Patch:
diff --git a/docs/examples/asyncio_examples.ipynb b/docs/examples/asyncio_examples.ipynb
index f7e67e2..a8ee8fa 100644
--- a/docs/examples/asyncio_examples.ipynb
+++ b/docs/examples/asyncio_examples.ipynb
@@ -44,6 +44,26 @@
     "await connection.close()"
    ]
   },
+  {
+   "cell_type": "markdown",
+   "metadata": {},
+   "source": [
+    "If you create custom `ConnectionPool` for the `Redis` instance to use alone, use the `from_pool` class method to create it.  This will cause the pool to be disconnected along with the Redis instance. Disconnecting the connection pool simply disconnects all connections hosted in the pool."
+   ]
+  },
+  {
+   "cell_type": "code",
+   "execution_count": null,
+   "metadata": {},
+   "outputs": [],
+   "source": [
+    "import redis.asyncio as redis\n",
+    "\n",
+    "pool = redis.ConnectionPool.from_url(\"redis://localhost\")\n",
+    "connection = redis.Redis.from_pool(pool)\n",
+    "await connection.close()"
+   ]
+  },
   {
    "cell_type": "markdown",
    "metadata": {
@@ -53,7 +73,8 @@
     }
    },
    "source": [
-    "If you supply a custom `ConnectionPool` that is supplied to several `Redis` instances, you may want to disconnect the connection pool explicitly. Disconnecting the connection pool simply disconnects all connections hosted in the pool."
+    "\n",
+    "However, If you supply a `ConnectionPool` that is shared several `Redis` instances, you may want to disconnect the connection pool explicitly. use the `connection_pool` argument in that case."
    ]
   },
   {
@@ -69,10 +90,12 @@
    "source": [
     "import redis.asyncio as redis\n",
     "\n",
-    "connection = redis.Redis(auto_close_connection_pool=False)\n",
-    "await connection.close()\n",
-    "# Or: await connection.close(close_connection_pool=False)\n",
-    "await connection.connection_pool.disconnect()"
+    "pool = redis.ConnectionPool.from_url(\"redis://localhost\")\n",
+    "connection1 = redis.Redis(connection_pool=pool)\n",
+    "connection2 = redis.Redis(connection_pool=pool)\n",
+    "await connection1.close()\n",
+    "await connection2.close()\n",
+    "await pool.disconnect()"
    ]
   },
   {
diff --git a/redis/asyncio/client.py b/redis/asyncio/client.py
index f0c1ab7..cce1b58 100644
--- a/redis/asyncio/client.py
+++ b/redis/asyncio/client.py
@@ -114,7 +114,7 @@ class Redis(
         cls,
         url: str,
         single_connection_client: bool = False,
-        auto_close_connection_pool: bool = True,
+        auto_close_connection_pool: Optional[bool] = None,
         **kwargs,
     ):
         """
@@ -160,12 +160,39 @@ class Redis(
 
         """
         connection_pool = ConnectionPool.from_url(url, **kwargs)
-        redis = cls(
+        client = cls(
             connection_pool=connection_pool,
             single_connection_client=single_connection_client,
         )
-        redis.auto_close_connection_pool = auto_close_connection_pool
-        return redis
+        if auto_close_connection_pool is not None:
+            warnings.warn(
+                DeprecationWarning(
+                    '"auto_close_connection_pool" is deprecated '
+                    "since version 5.0.0. "
+                    "Please create a ConnectionPool explicitly and "
+                    "provide to the Redis() constructor instead."
+                )
+            )
+        else:
+            auto_close_connection_pool = True
+        client.auto_close_connection_pool = auto_close_connection_pool
+        return client
+
+    @classmethod
+    def from_pool(
+        cls: Type["Redis"],
+        connection_pool: ConnectionPool,
+    ) -> "Redis":
+        """
+        Return a Redis client from the given connection pool.
+        The Redis client will take ownership of the connection pool and
+        close it when the Redis client is closed.
+        """
+        client = cls(
+            connection_pool=connection_pool,
+        )
+        client.auto_close_connection_pool = True
+        return client
 
     def __init__(
         self,
@@ -200,7 +227,8 @@ class Redis(
         lib_version: Optional[str] = get_lib_version(),
         username: Optional[str] = None,
         retry: Optional[Retry] = None,
-        auto_close_connection_pool: bool = True,
+        # deprecated. create a pool and use connection_pool instead
+        auto_close_connection_pool: Optional[bool] = None,
         redis_connect_func=None,
         credential_provider: Optional[CredentialProvider] = None,
         protocol: Optional[int] = 2,
@@ -213,14 +241,21 @@ class Redis(
         To retry on TimeoutError, `retry_on_timeout` can also be set to `True`.
         """
         kwargs: Dict[str, Any]
-        # auto_close_connection_pool only has an effect if connection_pool is
-        # None. This is a similar feature to the missing __del__ to resolve #1103,
-        # but it accounts for whether a user wants to manually close the connection
-        # pool, as a similar feature to ConnectionPool's __del__.
-        self.auto_close_connection_pool = (
-            auto_close_connection_pool if connection_pool is None else False
-        )
+
+        if auto_close_connection_pool is not None:
+            warnings.warn(
+                DeprecationWarning(
+                    '"auto_close_connection_pool" is deprecated '
+                    "since version 5.0.0. "
+                    "Please create a ConnectionPool explicitly and "
+                    "provide to the Redis() constructor instead."
+                )
+            )
+        else:
+            auto_close_connection_pool = True
+
         if not connection_pool:
+            # Create internal connection pool, expected to be closed by Redis instance
             if not retry_on_error:
                 retry_on_error = []
             if retry_on_timeout is True:
@@ -277,7 +312,13 @@ class Redis(
                             "ssl_check_hostname": ssl_check_hostname,
                         }
                     )
+            # This arg only used if no pool is passed in
+            self.auto_close_connection_pool = auto_close_connection_pool
             connection_pool = ConnectionPool(**kwargs)
+        else:
+            # If a pool is passed in, do not close it
+            self.auto_close_connection_pool = False
+
         self.connection_pool = connection_pool
         self.single_connection_client = single_connection_client
         self.connection: Optional[Connection] = None
diff --git a/redis/asyncio/cluster.py b/redis/asyncio/cluster.py
index 8440711..65e1bd0 100644
--- a/redis/asyncio/cluster.py
+++ b/redis/asyncio/cluster.py
@@ -588,13 +588,13 @@ class RedisCluster(AbstractRedis, AbstractRedisCluster, AsyncRedisClusterCommand
         # EVAL/EVALSHA.
         # - issue: https://github.com/redis/redis/issues/9493
         # - fix: https://github.com/redis/redis/pull/9733
-        if command in ("EVAL", "EVALSHA"):
+        if command.upper() in ("EVAL", "EVALSHA"):
             # command syntax: EVAL "script body" num_keys ...
             if len(args) < 2:
                 raise RedisClusterException(
                     f"Invalid args in command: {command, *args}"
                 )
-            keys = args[2 : 2 + args[1]]
+            keys = args[2 : 2 + int(args[1])]
             # if there are 0 keys, that means the script can be run on any node
             # so we can just return a random slot
             if not keys:
@@ -604,7 +604,7 @@ class RedisCluster(AbstractRedis, AbstractRedisCluster, AsyncRedisClusterCommand
             if not keys:
                 # FCALL can call a function with 0 keys, that means the function
                 #  can be run on any node so we can just return a random slot
-                if command in ("FCALL", "FCALL_RO"):
+                if command.upper() in ("FCALL", "FCALL_RO"):
                     return random.randrange(0, REDIS_CLUSTER_HASH_SLOTS)
                 raise RedisClusterException(
                     "No way to dispatch this command to Redis Cluster. "
diff --git a/redis/asyncio/connection.py b/redis/asyncio/connection.py
index 0086551..4f20e6e 100644
--- a/redis/asyncio/connection.py
+++ b/redis/asyncio/connection.py
@@ -1106,8 +1106,8 @@ class BlockingConnectionPool(ConnectionPool):
     """
     A blocking connection pool::
 
-        >>> from redis.asyncio.client import Redis
-        >>> client = Redis(connection_pool=BlockingConnectionPool())
+        >>> from redis.asyncio import Redis, BlockingConnectionPool
+        >>> client = Redis.from_pool(BlockingConnectionPool())
 
     It performs the same function as the default
     :py:class:`~redis.asyncio.ConnectionPool` implementation, in that,
diff --git a/redis/asyncio/sentinel.py b/redis/asyncio/sentinel.py
index 20c0a1f..6834fb1 100644
--- a/redis/asyncio/sentinel.py
+++ b/redis/asyncio/sentinel.py
@@ -340,12 +340,7 @@ class Sentinel(AsyncSentinelCommands):
 
         connection_pool = connection_pool_class(service_name, self, **connection_kwargs)
         # The Redis object "owns" the pool
-        auto_close_connection_pool = True
-        client = redis_class(
-            connection_pool=connection_pool,
-        )
-        client.auto_close_connection_pool = auto_close_connection_pool
-        return client
+        return redis_class.from_pool(connection_pool)
 
     def slave_for(
         self,
@@ -377,9 +372,4 @@ class Sentinel(AsyncSentinelCommands):
 
         connection_pool = connection_pool_class(service_name, self, **connection_kwargs)
         # The Redis object "owns" the pool
-        auto_close_connection_pool = True
-        client = redis_class(
-            connection_pool=connection_pool,
-        )
-        client.auto_close_connection_pool = auto_close_connection_pool
-        return client
+        return redis_class.from_pool(connection_pool)
diff --git a/redis/client.py b/redis/client.py
index f695cef..017f8ef 100755
--- a/redis/client.py
+++ b/redis/client.py
@@ -4,7 +4,7 @@ import threading
 import time
 import warnings
 from itertools import chain
-from typing import Optional
+from typing import Optional, Type
 
 from redis._parsers.helpers import (
     _RedisCallbacks,
@@ -136,10 +136,28 @@ class Redis(RedisModuleCommands, CoreCommands, SentinelCommands):
         """
         single_connection_client = kwargs.pop("single_connection_client", False)
         connection_pool = ConnectionPool.from_url(url, **kwargs)
-        return cls(
+        client = cls(
             connection_pool=connection_pool,
             single_connection_client=single_connection_client,
         )
+        client.auto_close_connection_pool = True
+        return client
+
+    @classmethod
+    def from_pool(
+        cls: Type["Redis"],
+        connection_pool: ConnectionPool,
+    ) -> "Redis":
+        """
+        Return a Redis client from the given connection pool.
+        The Redis client will take ownership of the connection pool and
+        close it when the Redis client is closed.
+        """
+        client = cls(
+            connection_pool=connection_pool,
+        )
+        client.auto_close_connection_pool = True
+        return client
 
     def __init__(
         self,
@@ -275,6 +293,10 @@ class Redis(RedisModuleCommands, CoreCommands, SentinelCommands):
                         }
                     )
             connection_pool = ConnectionPool(**kwargs)
+            self.auto_close_connection_pool = True
+        else:
+            self.auto_close_connection_pool = False
+
         self.connection_pool = connection_pool
         self.connection = None
         if single_connection_client:
@@ -477,6 +499,9 @@ class Redis(RedisModuleCommands, CoreCommands, SentinelCommands):
             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
diff --git a/redis/cluster.py b/redis/cluster.py
index cba62de..0347893 100644
--- a/redis/cluster.py
+++ b/redis/cluster.py
@@ -967,11 +967,11 @@ class RedisCluster(AbstractRedisCluster, RedisClusterCommands):
         # redis server to parse the keys. Besides, there is a bug in redis<7.0
         # where `self._get_command_keys()` fails anyway. So, we special case
         # EVAL/EVALSHA.
-        if command in ("EVAL", "EVALSHA"):
+        if command.upper() in ("EVAL", "EVALSHA"):
             # command syntax: EVAL "script body" num_keys ...
             if len(args) <= 2:
                 raise RedisClusterException(f"Invalid args in command: {args}")
-            num_actual_keys = args[2]
+            num_actual_keys = int(args[2])
             eval_keys = args[3 : 3 + num_actual_keys]
             # if there are 0 keys, that means the script can be run on any node
             # so we can just return a random slot
@@ -983,7 +983,7 @@ class RedisCluster(AbstractRedisCluster, RedisClusterCommands):
             if keys is None or len(keys) == 0:
                 # FCALL can call a function with 0 keys, that means the function
                 #  can be run on any node so we can just return a random slot
-                if command in ("FCALL", "FCALL_RO"):
+                if command.upper() in ("FCALL", "FCALL_RO"):
                     return random.randrange(0, REDIS_CLUSTER_HASH_SLOTS)
                 raise RedisClusterException(
                     "No way to dispatch this command to Redis Cluster. "
diff --git a/redis/sentinel.py b/redis/sentinel.py
index 836e781..41f308d 100644
--- a/redis/sentinel.py
+++ b/redis/sentinel.py
@@ -353,10 +353,8 @@ class Sentinel(SentinelCommands):
         kwargs["is_master"] = True
         connection_kwargs = dict(self.connection_kwargs)
         connection_kwargs.update(kwargs)
-        return redis_class(
-            connection_pool=connection_pool_class(
-                service_name, self, **connection_kwargs
-            )
+        return redis_class.from_pool(
+            connection_pool_class(service_name, self, **connection_kwargs)
         )
 
     def slave_for(
@@ -386,8 +384,6 @@ class Sentinel(SentinelCommands):
         kwargs["is_master"] = False
         connection_kwargs = dict(self.connection_kwargs)
         connection_kwargs.update(kwargs)
-        return redis_class(
-            connection_pool=connection_pool_class(
-                service_name, self, **connection_kwargs
-            )
+        return redis_class.from_pool(
+            connection_pool_class(service_name, self, **connection_kwargs)
         )


Test patch:
diff --git a/tests/test_asyncio/test_connection.py b/tests/test_asyncio/test_connection.py
index 9ee6db1..a954a40 100644
--- a/tests/test_asyncio/test_connection.py
+++ b/tests/test_asyncio/test_connection.py
@@ -11,7 +11,7 @@ from redis._parsers import (
     _AsyncRESP3Parser,
     _AsyncRESPBase,
 )
-from redis.asyncio import Redis
+from redis.asyncio import ConnectionPool, Redis
 from redis.asyncio.connection import Connection, UnixDomainSocketConnection, parse_url
 from redis.asyncio.retry import Retry
 from redis.backoff import NoBackoff
@@ -288,7 +288,7 @@ def test_create_single_connection_client_from_url():
     assert client.single_connection_client is True
 
 
-@pytest.mark.parametrize("from_url", (True, False))
+@pytest.mark.parametrize("from_url", (True, False), ids=("from_url", "from_args"))
 async def test_pool_auto_close(request, from_url):
     """Verify that basic Redis instances have auto_close_connection_pool set to True"""
 
@@ -305,20 +305,116 @@ async def test_pool_auto_close(request, from_url):
     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"""
+async def test_pool_from_url_deprecation(request):
+    url: str = request.config.getoption("--redis-url")
+
+    with pytest.deprecated_call():
+        return Redis.from_url(url, auto_close_connection_pool=False)
+
+
+async def test_pool_auto_close_disable(request):
+    """Verify that auto_close_connection_pool can be disabled (deprecated)"""
 
     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)
+        with pytest.deprecated_call():
+            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()
+
+
+@pytest.mark.parametrize("from_url", (True, False), ids=("from_url", "from_args"))
+async 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
+
+    async 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
+
+    async def mock_disconnect(_):
+        nonlocal called
+        called += 1
+
+    with patch.object(ConnectionPool, "disconnect", mock_disconnect):
+        async with await get_redis_connection() as r1:
+            assert r1.auto_close_connection_pool is False
+
+    assert called == 0
+    await pool.disconnect()
+
+
+@pytest.mark.parametrize("from_url", (True, False), ids=("from_url", "from_args"))
+async def test_redis_from_pool(request, from_url):
+    """Verify that basic Redis instances created 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
+
+    async 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
+
+    async def mock_disconnect(_):
+        nonlocal called
+        called += 1
+
+    with patch.object(ConnectionPool, "disconnect", mock_disconnect):
+        async with await get_redis_connection() as r1:
+            assert r1.auto_close_connection_pool is True
+
+    assert called == 1
+    await pool.disconnect()
+
+
+@pytest.mark.parametrize("auto_close", (True, False))
+async def test_redis_pool_auto_close_arg(request, auto_close):
+    """test that redis instance where pool is provided have
+    auto_close_connection_pool set to False, regardless of arg"""
+
+    url: str = request.config.getoption("--redis-url")
+    pool = ConnectionPool.from_url(url)
+
+    async def get_redis_connection():
+        with pytest.deprecated_call():
+            client = Redis(connection_pool=pool, auto_close_connection_pool=auto_close)
+        return client
+
+    called = 0
+
+    async def mock_disconnect(_):
+        nonlocal called
+        called += 1
+
+    with patch.object(ConnectionPool, "disconnect", mock_disconnect):
+        async with await get_redis_connection() as r1:
+            assert r1.auto_close_connection_pool is False
+
+    assert called == 0
+    await pool.disconnect()
diff --git a/tests/test_connection.py b/tests/test_connection.py
index 760b23c..bff2495 100644
--- a/tests/test_connection.py
+++ b/tests/test_connection.py
@@ -5,9 +5,15 @@ from unittest.mock import patch
 
 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 created 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()
diff --git a/tests/test_sentinel.py b/tests/test_sentinel.py
index b7bcc27..54b9647 100644
--- a/tests/test_sentinel.py
+++ b/tests/test_sentinel.py
@@ -1,4 +1,5 @@
 import socket
+from unittest import mock
 
 import pytest
 import redis.sentinel
@@ -240,3 +241,28 @@ def test_flushconfig(cluster, sentinel):
 def test_reset(cluster, sentinel):
     cluster.master["is_odown"] = True
     assert sentinel.sentinel_reset("mymaster")
+
+
+@pytest.mark.onlynoncluster
+@pytest.mark.parametrize("method_name", ["master_for", "slave_for"])
+def test_auto_close_pool(cluster, sentinel, method_name):
+    """
+    Check that the connection pool created by the sentinel client is
+    automatically closed
+    """
+
+    method = getattr(sentinel, method_name)
+    client = method("mymaster", db=9)
+    pool = client.connection_pool
+    assert client.auto_close_connection_pool is True
+    calls = 0
+
+    def mock_disconnect():
+        nonlocal calls
+        calls += 1
+
+    with mock.patch.object(pool, "disconnect", mock_disconnect):
+        client.close()
+
+    assert calls == 1
+    pool.disconnect()