From 3c06bcf2fd6e016f5d75ae7f11fec806d83c6488 Mon Sep 17 00:00:00 2001 From: "copilot-swe-agent[bot]" <198982749+Copilot@users.noreply.github.com> Date: Mon, 15 Dec 2025 15:12:05 +0000 Subject: [PATCH 1/4] Initial plan From 0172e3285a828b96b8b2a79377ef362c100e0d1d Mon Sep 17 00:00:00 2001 From: "copilot-swe-agent[bot]" <198982749+Copilot@users.noreply.github.com> Date: Mon, 15 Dec 2025 15:18:34 +0000 Subject: [PATCH 2/4] Fix async connection pool lock performance issue Move ensure_connection() outside the lock in ConnectionPool.get_connection() to prevent lock contention during socket connection and TLS handshake operations. This matches the pattern already used in BlockingConnectionPool and significantly improves performance under high load. Co-authored-by: petyaslavova <194077574+petyaslavova@users.noreply.github.com> --- redis/asyncio/connection.py | 15 ++-- tests/test_asyncio/test_connection_pool.py | 87 ++++++++++++++++++++++ 2 files changed, 95 insertions(+), 7 deletions(-) diff --git a/redis/asyncio/connection.py b/redis/asyncio/connection.py index c13b921e2b..e3008d91d2 100644 --- a/redis/asyncio/connection.py +++ b/redis/asyncio/connection.py @@ -1215,16 +1215,17 @@ def can_get_connection(self) -> bool: version="5.3.0", ) async def get_connection(self, command_name=None, *keys, **options): + """Get a connected connection from the pool""" async with self._lock: - """Get a connected connection from the pool""" connection = self.get_available_connection() - try: - await self.ensure_connection(connection) - except BaseException: - await self.release(connection) - raise - return connection + # We now perform the connection check outside of the lock. + try: + await self.ensure_connection(connection) + return connection + except BaseException: + await self.release(connection) + raise def get_available_connection(self): """Get a connection from the pool, without making sure it is connected""" diff --git a/tests/test_asyncio/test_connection_pool.py b/tests/test_asyncio/test_connection_pool.py index e658c14188..969fc85ca8 100644 --- a/tests/test_asyncio/test_connection_pool.py +++ b/tests/test_asyncio/test_connection_pool.py @@ -222,6 +222,93 @@ async def test_pool_disconnect(self, master_host): await pool.disconnect(inuse_connections=False) assert conn.is_connected + async def test_lock_not_held_during_connection_establishment(self): + """ + Test that the connection pool lock is not held during the + ensure_connection call, which involves socket connection and handshake. + This is important for performance under high load. + """ + + class SlowConnectConnection(DummyConnection): + """Connection that simulates slow connection establishment""" + + def __init__(self, **kwargs): + super().__init__(**kwargs) + self.connect_called = False + self.lock_held_during_connect = None + + async def connect(self): + self.connect_called = True + # Check if the pool's lock is held during connection + pool = self.kwargs.get("_pool") + if pool: + self.lock_held_during_connect = pool._lock.locked() + # Simulate slow connection + await asyncio.sleep(0.01) + self._connected = True + + async with self.get_pool( + connection_class=SlowConnectConnection, + connection_kwargs={"_pool": None}, + ) as pool: + # Pass the pool to the connection so it can check lock status + pool.connection_kwargs["_pool"] = pool + + # Get a connection - this should call connect() outside the lock + connection = await pool.get_connection() + + # Verify connect was called + assert connection.connect_called + + # Verify the lock was NOT held during connect + assert ( + connection.lock_held_during_connect is False + ), "Lock should not be held during connection establishment" + + await pool.release(connection) + + async def test_concurrent_connection_acquisition_performance(self): + """ + Test that multiple concurrent connection acquisitions don't block + each other during connection establishment. + """ + + class SlowConnectConnection(DummyConnection): + """Connection that simulates slow connection establishment""" + + async def connect(self): + # Simulate slow connection (e.g., network latency, TLS handshake) + await asyncio.sleep(0.05) + self._connected = True + + async with self.get_pool( + connection_class=SlowConnectConnection, max_connections=10 + ) as pool: + # Start acquiring multiple connections concurrently + start_time = asyncio.get_running_loop().time() + + # Try to get 3 connections concurrently + connections = await asyncio.gather( + pool.get_connection(), + pool.get_connection(), + pool.get_connection(), + ) + + elapsed_time = asyncio.get_running_loop().time() - start_time + + # With proper lock handling, these should complete mostly in parallel + # If the lock was held during connect(), it would take 3 * 0.05 = 0.15s + # With lock only during pop, it should take ~0.05s (connections in parallel) + # We allow some overhead, so check it's less than 0.12s + assert elapsed_time < 0.12, ( + f"Concurrent connections took {elapsed_time}s, " + f"suggesting lock was held during connection establishment" + ) + + # Clean up + for conn in connections: + await pool.release(conn) + class TestBlockingConnectionPool: @asynccontextmanager From cc13a45c706cde5f7b0167e54b7d939f94c614f1 Mon Sep 17 00:00:00 2001 From: "copilot-swe-agent[bot]" <198982749+Copilot@users.noreply.github.com> Date: Mon, 15 Dec 2025 15:20:35 +0000 Subject: [PATCH 3/4] Improve test design based on code review feedback - Simplify lock checking test by using outer scope instead of passing pool reference - Make performance test more maintainable with configurable timeouts and better error messages Co-authored-by: petyaslavova <194077574+petyaslavova@users.noreply.github.com> --- tests/test_asyncio/test_connection_pool.py | 50 ++++++++-------------- 1 file changed, 19 insertions(+), 31 deletions(-) diff --git a/tests/test_asyncio/test_connection_pool.py b/tests/test_asyncio/test_connection_pool.py index 969fc85ca8..48ade3872f 100644 --- a/tests/test_asyncio/test_connection_pool.py +++ b/tests/test_asyncio/test_connection_pool.py @@ -228,41 +228,27 @@ async def test_lock_not_held_during_connection_establishment(self): ensure_connection call, which involves socket connection and handshake. This is important for performance under high load. """ + lock_states = [] class SlowConnectConnection(DummyConnection): """Connection that simulates slow connection establishment""" - def __init__(self, **kwargs): - super().__init__(**kwargs) - self.connect_called = False - self.lock_held_during_connect = None - async def connect(self): - self.connect_called = True # Check if the pool's lock is held during connection - pool = self.kwargs.get("_pool") - if pool: - self.lock_held_during_connect = pool._lock.locked() + # We access the pool through the outer scope + lock_states.append(pool._lock.locked()) # Simulate slow connection await asyncio.sleep(0.01) self._connected = True - async with self.get_pool( - connection_class=SlowConnectConnection, - connection_kwargs={"_pool": None}, - ) as pool: - # Pass the pool to the connection so it can check lock status - pool.connection_kwargs["_pool"] = pool - + async with self.get_pool(connection_class=SlowConnectConnection) as pool: # Get a connection - this should call connect() outside the lock connection = await pool.get_connection() - # Verify connect was called - assert connection.connect_called - # Verify the lock was NOT held during connect + assert len(lock_states) > 0, "connect() should have been called" assert ( - connection.lock_held_during_connect is False + lock_states[0] is False ), "Lock should not be held during connection establishment" await pool.release(connection) @@ -272,13 +258,15 @@ async def test_concurrent_connection_acquisition_performance(self): Test that multiple concurrent connection acquisitions don't block each other during connection establishment. """ + connection_delay = 0.05 + num_connections = 3 class SlowConnectConnection(DummyConnection): """Connection that simulates slow connection establishment""" async def connect(self): # Simulate slow connection (e.g., network latency, TLS handshake) - await asyncio.sleep(0.05) + await asyncio.sleep(connection_delay) self._connected = True async with self.get_pool( @@ -287,22 +275,22 @@ async def connect(self): # Start acquiring multiple connections concurrently start_time = asyncio.get_running_loop().time() - # Try to get 3 connections concurrently + # Try to get connections concurrently connections = await asyncio.gather( - pool.get_connection(), - pool.get_connection(), - pool.get_connection(), + *[pool.get_connection() for _ in range(num_connections)] ) elapsed_time = asyncio.get_running_loop().time() - start_time # With proper lock handling, these should complete mostly in parallel - # If the lock was held during connect(), it would take 3 * 0.05 = 0.15s - # With lock only during pop, it should take ~0.05s (connections in parallel) - # We allow some overhead, so check it's less than 0.12s - assert elapsed_time < 0.12, ( - f"Concurrent connections took {elapsed_time}s, " - f"suggesting lock was held during connection establishment" + # If the lock was held during connect(), it would take num_connections * connection_delay + # With lock only during pop, it should take ~connection_delay (connections in parallel) + # We allow 2.5x overhead for system variance + max_allowed_time = connection_delay * 2.5 + assert elapsed_time < max_allowed_time, ( + f"Concurrent connections took {elapsed_time:.3f}s, " + f"expected < {max_allowed_time:.3f}s. " + f"This suggests lock was held during connection establishment." ) # Clean up From 71040553a99064e9c7a15d888339b2f11be1c27c Mon Sep 17 00:00:00 2001 From: "copilot-swe-agent[bot]" <198982749+Copilot@users.noreply.github.com> Date: Mon, 15 Dec 2025 15:55:03 +0000 Subject: [PATCH 4/4] Fix linter formatting error in test file Co-authored-by: petyaslavova <194077574+petyaslavova@users.noreply.github.com> --- tests/test_asyncio/test_connection_pool.py | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/tests/test_asyncio/test_connection_pool.py b/tests/test_asyncio/test_connection_pool.py index 48ade3872f..0316ff8797 100644 --- a/tests/test_asyncio/test_connection_pool.py +++ b/tests/test_asyncio/test_connection_pool.py @@ -247,9 +247,9 @@ async def connect(self): # Verify the lock was NOT held during connect assert len(lock_states) > 0, "connect() should have been called" - assert ( - lock_states[0] is False - ), "Lock should not be held during connection establishment" + assert lock_states[0] is False, ( + "Lock should not be held during connection establishment" + ) await pool.release(connection)