From 9fa5bf23f02d7674709c58c91ae81a8d65b3cbb0 Mon Sep 17 00:00:00 2001 From: taras Date: Sun, 19 Jul 2026 19:01:10 +0300 Subject: [PATCH] ConnectionPool: release the claimed slot when the factory throws MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit pop() (and fill()) claim a pool slot with an atomic cmpset BEFORE calling the connection factory, but the factory is a real PDO connect that can throw — a transient network blip, MySQL momentarily refusing, an auth hiccup. When it threw, the exception propagated out with the slot still counted: nothing anywhere decrements `created` except full teardown. Each failed connect therefore permanently burned a slot. After `size` such failures the pool was full-but-empty forever: the fast-path guard `isEmpty() && current < size` is never true again, so every pop() falls through to `Channel->pop($timeout)` with the default timeout of -1 and blocks its caller indefinitely, waiting for a connection that will never be pushed. A ratcheting deadlock that only a restart cleared. This took down production: over ~4 days of uptime the busiest (default) connection accumulated slot leaks until all Swoole workers were asleep in ConnectionPool::pop() and every request hung — while MySQL itself was healthy and nowhere near its connection limit, because the "held" connections were phantom slots that were never actually opened. Fix: wrap the factory call in try/catch and roll back the cmpset with `created->sub(1)` before rethrowing, in both pop() and fill(). A failed connect now leaves the slot free for the next attempt. Adds a regression test that drives repeated factory failures and asserts the slot counter never leaks and the pool stays usable once the factory recovers. The test fails fast (not hangs) against the unpatched code. Co-Authored-By: Claude Opus 4.8 (1M context) --- src/Adapter/ConnectionPool.php | 28 +++++++++++- tests/Unit/Adapter/ConnectionPoolTest.php | 52 +++++++++++++++++++++++ 2 files changed, 78 insertions(+), 2 deletions(-) diff --git a/src/Adapter/ConnectionPool.php b/src/Adapter/ConnectionPool.php index aa6ea5d..c406b44 100644 --- a/src/Adapter/ConnectionPool.php +++ b/src/Adapter/ConnectionPool.php @@ -89,7 +89,22 @@ public function pop(float $timeout = -1): \PDO if ($this->created->cmpset($current, $current + 1)) { // We won the race — create and return the new connection // without pushing it to the channel first. - return ($this->factory)(); + try { + return ($this->factory)(); + } catch (\Throwable $e) { + // The factory (a real PDO connect) can throw: a transient + // network blip, MySQL momentarily refusing, an auth hiccup. + // The slot was already claimed by the cmpset above, so it + // MUST be released here — nothing else ever decrements + // `created`. Without this rollback every failed connect + // permanently burns a slot; after `size` failures the pool + // is full-but-empty forever, so pop() falls through to the + // Channel->pop() below and (with timeout -1) blocks every + // caller indefinitely. That ratcheting deadlock took down + // production with all workers asleep in pop(). + $this->created->sub(1); + throw $e; + } } } @@ -150,7 +165,16 @@ public function fill(): void while ($current < $this->size) { if ($this->created->cmpset($current, $current + 1)) { - $pool->push(($this->factory)()); + // Same slot-leak guard as pop(): if the factory throws (DB not + // yet reachable at worker boot) release the claimed slot before + // propagating, so a later retry/pop() can still fill the pool + // instead of it being permanently short one connection. + try { + $pool->push(($this->factory)()); + } catch (\Throwable $e) { + $this->created->sub(1); + throw $e; + } } $current = $this->created->get(); diff --git a/tests/Unit/Adapter/ConnectionPoolTest.php b/tests/Unit/Adapter/ConnectionPoolTest.php index 8e31629..cbd111f 100644 --- a/tests/Unit/Adapter/ConnectionPoolTest.php +++ b/tests/Unit/Adapter/ConnectionPoolTest.php @@ -139,6 +139,58 @@ public function it_resets_state_even_on_cleanup_error(): void }); } + #[Test] + public function pop_releases_the_claimed_slot_when_the_factory_throws(): void + { + if (!class_exists(\Swoole\Coroutine::class)) { + self::markTestSkipped('Swoole extension is required.'); + } + + Coroutine\run(function () { + $fail = true; + $pool = new ConnectionPool(1, static function () use (&$fail): \PDO { + if ($fail) { + // Simulate a transient PDO connect failure — a network blip + // or MySQL momentarily refusing the connection. + throw new \PDOException('simulated connect failure'); + } + return new \PDO('sqlite::memory:'); + }); + + $ref = new \ReflectionClass($pool); + $createdProp = $ref->getProperty('created'); + $createdProp->setAccessible(true); + + // Repeated connect failures must NOT accumulate on the slot counter. + // Before the fix each failure leaked one slot (cmpset ran, the factory + // threw, nothing rolled it back); after `size` failures the pool was + // full-but-empty and every pop() blocked forever on the Channel — the + // production ratcheting deadlock this guards against. + for ($i = 0; $i < 5; ++$i) { + try { + $pool->pop(); + self::fail('Expected the factory exception to propagate.'); + } catch (\PDOException $e) { + self::assertSame('simulated connect failure', $e->getMessage()); + } + + self::assertSame( + 0, + $createdProp->getValue($pool)->get(), + 'A failed factory call must release the slot it optimistically claimed.', + ); + } + + // Not wedged: once the factory recovers, pop() creates a real + // connection via the fast path instead of blocking on an empty + // Channel (which, with timeout -1, would hang forever). + $fail = false; + $conn = $pool->pop(); + self::assertInstanceOf(\PDO::class, $conn); + self::assertSame(1, $createdProp->getValue($pool)->get()); + }); + } + protected function tearDown(): void { if (class_exists(\Swoole\Runtime::class)) {