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.

diff --git a/docs/news_pool.rst b/docs/news_pool.rst
index fc1e798b86dc471715c7fa4639ab435f696e1f58..332331ec9428ea27479adfe0c9d915758e1e85bf 100644 (file)
--- 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 156f496cc37327241ff85c79b170af8cc041a0b0..63e7e5796de444812e151e447215562d012df5c2 100644 (file)
--- 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 5ae43844cfbdfa1ef22e786dc702adc25eeac195..36ca412bd5e731eceb9d6c6a3f7222b280225dfb 100644 (file)
--- 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: