Skip to content

Commit 0172e32

Browse files
Copilotpetyaslavova
andcommitted
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>
1 parent 3c06bcf commit 0172e32

File tree

2 files changed

+95
-7
lines changed

2 files changed

+95
-7
lines changed

redis/asyncio/connection.py

Lines changed: 8 additions & 7 deletions
Original file line numberDiff line numberDiff line change
@@ -1215,16 +1215,17 @@ def can_get_connection(self) -> bool:
12151215
version="5.3.0",
12161216
)
12171217
async def get_connection(self, command_name=None, *keys, **options):
1218+
"""Get a connected connection from the pool"""
12181219
async with self._lock:
1219-
"""Get a connected connection from the pool"""
12201220
connection = self.get_available_connection()
1221-
try:
1222-
await self.ensure_connection(connection)
1223-
except BaseException:
1224-
await self.release(connection)
1225-
raise
12261221

1227-
return connection
1222+
# We now perform the connection check outside of the lock.
1223+
try:
1224+
await self.ensure_connection(connection)
1225+
return connection
1226+
except BaseException:
1227+
await self.release(connection)
1228+
raise
12281229

12291230
def get_available_connection(self):
12301231
"""Get a connection from the pool, without making sure it is connected"""

tests/test_asyncio/test_connection_pool.py

Lines changed: 87 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -222,6 +222,93 @@ async def test_pool_disconnect(self, master_host):
222222
await pool.disconnect(inuse_connections=False)
223223
assert conn.is_connected
224224

225+
async def test_lock_not_held_during_connection_establishment(self):
226+
"""
227+
Test that the connection pool lock is not held during the
228+
ensure_connection call, which involves socket connection and handshake.
229+
This is important for performance under high load.
230+
"""
231+
232+
class SlowConnectConnection(DummyConnection):
233+
"""Connection that simulates slow connection establishment"""
234+
235+
def __init__(self, **kwargs):
236+
super().__init__(**kwargs)
237+
self.connect_called = False
238+
self.lock_held_during_connect = None
239+
240+
async def connect(self):
241+
self.connect_called = True
242+
# Check if the pool's lock is held during connection
243+
pool = self.kwargs.get("_pool")
244+
if pool:
245+
self.lock_held_during_connect = pool._lock.locked()
246+
# Simulate slow connection
247+
await asyncio.sleep(0.01)
248+
self._connected = True
249+
250+
async with self.get_pool(
251+
connection_class=SlowConnectConnection,
252+
connection_kwargs={"_pool": None},
253+
) as pool:
254+
# Pass the pool to the connection so it can check lock status
255+
pool.connection_kwargs["_pool"] = pool
256+
257+
# Get a connection - this should call connect() outside the lock
258+
connection = await pool.get_connection()
259+
260+
# Verify connect was called
261+
assert connection.connect_called
262+
263+
# Verify the lock was NOT held during connect
264+
assert (
265+
connection.lock_held_during_connect is False
266+
), "Lock should not be held during connection establishment"
267+
268+
await pool.release(connection)
269+
270+
async def test_concurrent_connection_acquisition_performance(self):
271+
"""
272+
Test that multiple concurrent connection acquisitions don't block
273+
each other during connection establishment.
274+
"""
275+
276+
class SlowConnectConnection(DummyConnection):
277+
"""Connection that simulates slow connection establishment"""
278+
279+
async def connect(self):
280+
# Simulate slow connection (e.g., network latency, TLS handshake)
281+
await asyncio.sleep(0.05)
282+
self._connected = True
283+
284+
async with self.get_pool(
285+
connection_class=SlowConnectConnection, max_connections=10
286+
) as pool:
287+
# Start acquiring multiple connections concurrently
288+
start_time = asyncio.get_running_loop().time()
289+
290+
# Try to get 3 connections concurrently
291+
connections = await asyncio.gather(
292+
pool.get_connection(),
293+
pool.get_connection(),
294+
pool.get_connection(),
295+
)
296+
297+
elapsed_time = asyncio.get_running_loop().time() - start_time
298+
299+
# With proper lock handling, these should complete mostly in parallel
300+
# If the lock was held during connect(), it would take 3 * 0.05 = 0.15s
301+
# With lock only during pop, it should take ~0.05s (connections in parallel)
302+
# We allow some overhead, so check it's less than 0.12s
303+
assert elapsed_time < 0.12, (
304+
f"Concurrent connections took {elapsed_time}s, "
305+
f"suggesting lock was held during connection establishment"
306+
)
307+
308+
# Clean up
309+
for conn in connections:
310+
await pool.release(conn)
311+
225312

226313
class TestBlockingConnectionPool:
227314
@asynccontextmanager

0 commit comments

Comments
 (0)