From aee0bf2659db77c31154acf583baf0a98675c192 Mon Sep 17 00:00:00 2001 From: Oliver Haas Date: Thu, 30 Apr 2026 09:52:16 +0200 Subject: [PATCH] fix(pool): fix race in the construction of the sync ConnectionPool lock The lock used to make `ConnectionPool.open()` thread-safe was itself constructed lazily in `_ensure_lock()` without any guard. Two threads racing the first `open()` could each create a different `Lock`, defeat the mutual exclusion in `_open()`, both pass the `_closed` guard, and both call `_start_workers()`. The second one tripped `assert not self._workers`. Construct the sync lock eagerly in `__init__`. The async lock must stay lazy because `asyncio.Lock` binds to the running event loop. --- docs/news_pool.rst | 2 ++ psycopg_pool/psycopg_pool/pool.py | 20 ++++------------ psycopg_pool/psycopg_pool/pool_async.py | 32 +++++++++++++++---------- 3 files changed, 26 insertions(+), 28 deletions(-) diff --git a/docs/news_pool.rst b/docs/news_pool.rst index fc1e798b8..332331ec9 100644 --- a/docs/news_pool.rst +++ b/docs/news_pool.rst @@ -15,6 +15,8 @@ psycopg_pool 3.3.1 (unreleased) - Fix residual race condition catching `~asyncio.CancelledError` on connection (:ticket:`#1275`). +- Fix race condition constructing the lock that makes sync + `~ConnectionPool.open()` thread-safe (:ticket:`#1300`). Current release diff --git a/psycopg_pool/psycopg_pool/pool.py b/psycopg_pool/psycopg_pool/pool.py index 156f496cc..63e7e5796 100644 --- a/psycopg_pool/psycopg_pool/pool.py +++ b/psycopg_pool/psycopg_pool/pool.py @@ -105,6 +105,10 @@ class ConnectionPool(Generic[CT], BasePool): num_workers=num_workers, ) + # Construct the lock during single-threaded `__init__` so that + # threads concurrently calling `open()` can't race on it. + self._lock = Lock() + if open is None: open = self._open_implicit = True @@ -380,8 +384,6 @@ class ConnectionPool(Generic[CT], BasePool): because the pool was initialized with *open* = `!True`) but you cannot currently re-open a closed pool. """ - # Make sure the lock is created after there is an event loop - self._ensure_lock() with self._lock: self._open() @@ -395,9 +397,6 @@ class ConnectionPool(Generic[CT], BasePool): self._check_open() - # A lock has been most likely, but not necessarily, created in `open()`. - self._ensure_lock() - # Create these objects now to attach them to the right loop. # See #219 self._tasks = Queue() @@ -409,17 +408,6 @@ class ConnectionPool(Generic[CT], BasePool): self._start_workers() self._start_initial_tasks() - def _ensure_lock(self) -> None: - """Make sure the pool lock is created. - - In async code, also make sure that the loop is running. - """ - - try: - self._lock - except AttributeError: - self._lock = Lock() - def _start_workers(self) -> None: self._sched_runner = spawn(self._sched.run, name=f"{self.name}-scheduler") assert not self._workers diff --git a/psycopg_pool/psycopg_pool/pool_async.py b/psycopg_pool/psycopg_pool/pool_async.py index 5ae43844c..36ca412bd 100644 --- a/psycopg_pool/psycopg_pool/pool_async.py +++ b/psycopg_pool/psycopg_pool/pool_async.py @@ -117,6 +117,10 @@ class AsyncConnectionPool(Generic[ACT], BasePool): if True: # ASYNC if open: self._warn_open_async() + else: + # Construct the lock during single-threaded `__init__` so that + # threads concurrently calling `open()` can't race on it. + self._lock = ALock() if open is None: open = self._open_implicit = True @@ -418,8 +422,9 @@ class AsyncConnectionPool(Generic[ACT], BasePool): because the pool was initialized with *open* = `!True`) but you cannot currently re-open a closed pool. """ - # Make sure the lock is created after there is an event loop - self._ensure_lock() + if True: # ASYNC + # Make sure the lock is created after there is an event loop + self._ensure_lock() async with self._lock: self._open() @@ -433,8 +438,10 @@ class AsyncConnectionPool(Generic[ACT], BasePool): self._check_open() - # A lock has been most likely, but not necessarily, created in `open()`. - self._ensure_lock() + if True: # ASYNC + # A lock has been most likely, but not necessarily, created + # in `open()`. The sync pool creates it in `__init__()`. + self._ensure_lock() # Create these objects now to attach them to the right loop. # See #219 @@ -447,12 +454,16 @@ class AsyncConnectionPool(Generic[ACT], BasePool): self._start_workers() self._start_initial_tasks() - def _ensure_lock(self) -> None: - """Make sure the pool lock is created. + if True: # ASYNC + + def _ensure_lock(self) -> None: + """Make sure the pool lock is created and the loop is running.""" + try: + self._lock + return + except AttributeError: + pass - In async code, also make sure that the loop is running. - """ - if True: # ASYNC try: asyncio.get_running_loop() except RuntimeError: @@ -460,9 +471,6 @@ class AsyncConnectionPool(Generic[ACT], BasePool): f"{type(self).__name__} open with no running loop" ) from None - try: - self._lock - except AttributeError: self._lock = ALock() def _start_workers(self) -> None: -- 2.47.3