From eeb8aed63f2569a42d5602a7b61f83229faabbb0 Mon Sep 17 00:00:00 2001 From: Petya Slavova Date: Tue, 18 Feb 2025 12:25:33 +0200 Subject: [PATCH 1/3] Removing decreasing of created connections count when releasing not owned by connection pool connection(#2832). --- redis/connection.py | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-) diff --git a/redis/connection.py b/redis/connection.py index d59a9b069b..00d78e4997 100644 --- a/redis/connection.py +++ b/redis/connection.py @@ -1540,10 +1540,10 @@ def release(self, connection: "Connection") -> None: AfterConnectionReleasedEvent(connection) ) else: - # pool doesn't own this connection. do not add it back - # to the pool and decrement the count so that another - # connection can take its place if needed - self._created_connections -= 1 + # Pool doesn't own this connection, do not add it back + # to the pool. + # The created connections count shouls not be changed, + # because the connection was not created by the pool. connection.disconnect() return From ec979f0178bc6043af3036f18dc4c22b7c4c7d21 Mon Sep 17 00:00:00 2001 From: Petya Slavova Date: Tue, 18 Feb 2025 15:05:35 +0200 Subject: [PATCH 2/3] Fixed another issue that was allowing adding connections to a pool owned by other pools. Adding unit tests. --- redis/connection.py | 2 +- tests/test_connection_pool.py | 15 +++++++++++++++ tests/test_multiprocessing.py | 34 ++++++++++++++++++++++++++++++++++ 3 files changed, 50 insertions(+), 1 deletion(-) diff --git a/redis/connection.py b/redis/connection.py index 00d78e4997..48c32ea5f8 100644 --- a/redis/connection.py +++ b/redis/connection.py @@ -1532,7 +1532,7 @@ def release(self, connection: "Connection") -> None: except KeyError: # Gracefully fail when a connection is returned to this pool # that the pool doesn't actually own - pass + return if self.owns_connection(connection): self._available_connections.append(connection) diff --git a/tests/test_connection_pool.py b/tests/test_connection_pool.py index 387a0f4565..65f42923fe 100644 --- a/tests/test_connection_pool.py +++ b/tests/test_connection_pool.py @@ -91,6 +91,21 @@ def test_reuse_previously_released_connection(self, master_host): c2 = pool.get_connection() assert c1 == c2 + def test_release_not_owned_connection(self, master_host): + connection_kwargs = {"host": master_host[0], "port": master_host[1]} + pool1 = self.get_pool(connection_kwargs=connection_kwargs) + c1 = pool1.get_connection("_") + pool2 = self.get_pool( + connection_kwargs={"host": master_host[0], "port": master_host[1]} + ) + c2 = pool2.get_connection("_") + pool2.release(c2) + + assert len(pool2._available_connections) == 1 + + pool2.release(c1) + assert len(pool2._available_connections) == 1 + def test_repr_contains_db_info_tcp(self): connection_kwargs = { "host": "localhost", diff --git a/tests/test_multiprocessing.py b/tests/test_multiprocessing.py index 0e8e8958c5..8b9e9fb90b 100644 --- a/tests/test_multiprocessing.py +++ b/tests/test_multiprocessing.py @@ -84,6 +84,40 @@ def target(conn, ev): proc.join(3) assert proc.exitcode == 0 + @pytest.mark.parametrize("max_connections", [2, None]) + def test_release_parent_connection_from_pool_in_child_process( + self, max_connections, master_host + ): + """ + A connection owned by a parent should not decrease the _created_connections + counter in child when released - when the child process starts to use the + pool it resets all the counters that have been set in the parent process. + """ + + pool = ConnectionPool.from_url( + f"redis://{master_host[0]}:{master_host[1]}", + max_connections=max_connections, + ) + + parent_conn = pool.get_connection("ping") + + def target(pool, parent_conn): + with exit_callback(pool.disconnect): + child_conn = pool.get_connection("ping") + assert child_conn.pid != parent_conn.pid + pool.release(child_conn) + assert pool._created_connections == 1 + assert child_conn in pool._available_connections + pool.release(parent_conn) + assert pool._created_connections == 1 + assert child_conn in pool._available_connections + assert parent_conn not in pool._available_connections + + proc = multiprocessing.Process(target=target, args=(pool, parent_conn)) + proc.start() + proc.join(3) + assert proc.exitcode == 0 + @pytest.mark.parametrize("max_connections", [1, 2, None]) def test_pool(self, max_connections, master_host): """ From ab674fd1672e56626ddae83ab9417e453f51a041 Mon Sep 17 00:00:00 2001 From: Petya Slavova Date: Mon, 24 Feb 2025 15:30:25 +0200 Subject: [PATCH 3/3] Fixing a typo in a comment --- redis/connection.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/redis/connection.py b/redis/connection.py index 48c32ea5f8..ece17d752b 100644 --- a/redis/connection.py +++ b/redis/connection.py @@ -1542,7 +1542,7 @@ def release(self, connection: "Connection") -> None: else: # Pool doesn't own this connection, do not add it back # to the pool. - # The created connections count shouls not be changed, + # The created connections count should not be changed, # because the connection was not created by the pool. connection.disconnect() return