Skip to content
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
28 changes: 26 additions & 2 deletions src/Adapter/ConnectionPool.php
Original file line number Diff line number Diff line change
Expand Up @@ -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);

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

P1 Badge Wake or retry waiters after releasing a failed slot

When a DB outage hits while more coroutines are calling pop() than the pool size, the extra callers can already be blocked in Channel->pop(-1) before the in-flight factory attempts throw. This rollback frees capacity, but it does not wake those waiters or make them re-check created < size, so if all factory attempts fail and no later request enters pop(), those existing requests remain asleep indefinitely even though the slot counter is back below the limit.

Useful? React with 👍 / 👎.

throw $e;
}
}
}

Expand Down Expand Up @@ -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();
Expand Down
52 changes: 52 additions & 0 deletions tests/Unit/Adapter/ConnectionPoolTest.php
Original file line number Diff line number Diff line change
Expand Up @@ -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)) {
Expand Down