Skip to content

Fix pool exhaustion: retry on creation failure, add diagnostics #29

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
abnegate merged 1 commit into from
Mar 11, 2026
Merged

Fix pool exhaustion: retry on creation failure, add diagnostics #29

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
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
14 changes: 11 additions & 3 deletions src/Pools/Pool.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -251,6 +251,7 @@ public function pop(): Connection
{
$attempts = 0;
$totalSleepTime = 0;
$lastException = null;

try {
do {
Expand Down Expand Up @@ -279,15 +280,20 @@ public function pop(): Connection
$this->pool->synchronized(function () {
$this->connectionsCreated--;
});
throw $e;
// Don't throw immediately - fall through to try getting
// an existing connection from the pool
$lastException = $e;
Comment on lines +283 to +285
Copy link

@coderabbitai coderabbitai bot Mar 11, 2026

Choose a reason for hiding this comment

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

⚠️ Potential issue | 🟡 Minor

The chained exception still drops the original cause.

$lastException here is only the wrapper returned by createConnection(). That method currently throws a fresh \Exception without $e as previous, so the final chain still loses the original exception type and stack.

Preserve the root cause from createConnection() 
             } catch (\Exception $e) {
                 if ($attempts >= $this->getReconnectAttempts()) {
-                    throw new \Exception('Failed to create connection: ' . $e->getMessage());
+                    throw new \Exception('Failed to create connection: ' . $e->getMessage(), 0, $e);
                 }
                 sleep($this->getReconnectSleep());
             }

Also applies to: 293-296, 311-313

🤖 Prompt for AI Agents 
Verify each finding against the current code and only fix it if needed.

In `@src/Pools/Pool.php` around lines 283 - 285, The exception chain is losing the
original cause because createConnection() throws a fresh \Exception without
passing the caught exception as the previous; update createConnection() to
rethrow its error with the original exception as the third parameter (previous)
so callers (and $lastException in Pool::$lastException assignments) retain the
root cause; alternatively, where Pool sets $lastException = $e after catching
the createConnection() wrapper, set $lastException to the original exception (or
use the wrapper's getPrevious()) so the final thrown exception preserves the
original type and stack; check the createConnection() method and the Pool code
around the $lastException assignments referenced (including the similar spots at
the other catch blocks).

}
}

$connection = $this->pool->pop($this->getSynchronizationTimeout());

if ($connection === false || $connection === null) {
if ($attempts >= $this->getRetryAttempts()) {
throw new Exception("Pool '{$this->name}' is empty (size {$this->size})");
$activeCount = count($this->active);
$idleCount = $this->pool->count();
$message = "Pool '{$this->name}' is empty (size {$this->size}, active {$activeCount}, idle {$idleCount})";
throw new Exception($message, 0, $lastException);
Comment on lines +293 to +296
Copy link

@coderabbitai coderabbitai bot Mar 11, 2026

Choose a reason for hiding this comment

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

⚠️ Potential issue | 🟠 Major

Don't report repeated creation failures as pool exhaustion.

If $init keeps failing and the pool never creates a connection, Lines 293-296 still end with Pool ... is empty (active 0, idle 0). That sends callers toward leak/capacity debugging even though the real failure is connection establishment. Please keep a distinct terminal message when $lastException !== null and no connection was ever obtained.

Possible fix 
                 if ($connection === false || $connection === null) {
                     if ($attempts >= $this->getRetryAttempts()) {
                         $activeCount = count($this->active);
                         $idleCount = $this->pool->count();
-                        $message = "Pool '{$this->name}' is empty (size {$this->size}, active {$activeCount}, idle {$idleCount})";
+                        $message = ($lastException !== null && $activeCount === 0 && $idleCount === 0)
+                            ? "Pool '{$this->name}' failed to create a connection (size {$this->size}, active {$activeCount}, idle {$idleCount})"
+                            : "Pool '{$this->name}' is empty (size {$this->size}, active {$activeCount}, idle {$idleCount})";
                         throw new Exception($message, 0, $lastException);
                     }
🤖 Prompt for AI Agents 
Verify each finding against the current code and only fix it if needed.

In `@src/Pools/Pool.php` around lines 293 - 296, When failing to report pool
exhaustion in Pool:: (around the throw at the end of the acquire path), detect
if $lastException !== null and there were zero active/idle connections and
instead surface a distinct terminal error about connection creation failure
(include $lastException details) rather than the generic "Pool ... is empty"
message; update the logic that currently builds $message and throws new
Exception($message, 0, $lastException) so that when $lastException is set you
either rethrow $lastException or throw a new Exception with a clear
creation-failure message (and attach $lastException as the previous exception)
to make the failure cause explicit.

}

$totalSleepTime += $this->getRetrySleep();
Expand All @@ -302,7 +308,9 @@ public function pop(): Connection
}
} while ($attempts < $this->getRetryAttempts());

throw new Exception('Failed to get a connection from the pool');
$activeCount = count($this->active);
$idleCount = $this->pool->count();
throw new Exception("Pool '{$this->name}' failed to provide a connection (size {$this->size}, active {$activeCount}, idle {$idleCount})", 0, $lastException);
} finally {
$this->recordPoolTelemetry();
$this->telemetryWaitDuration->record($totalSleepTime, $this->telemetryAttributes);
Expand Down
66 changes: 66 additions & 0 deletions tests/Pools/Scopes/PoolTestScope.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -318,6 +318,72 @@ public function testPoolDestroy(): void
});
}

public function testPopRetriesAfterConnectionCreationFailure(): void
{
$this->execute(function (): void {
$callCount = 0;
$pool = new Pool($this->getAdapter(), 'test-retry', 1, function () use (&$callCount) {
$callCount++;
if ($callCount <= 1) {
throw new \Exception('Connection failed');
}
return 'x';
});
$pool->setReconnectAttempts(1);
$pool->setReconnectSleep(0);
$pool->setRetrySleep(0);

// With the fix, pop() should retry after creation failure
// First attempt: createConnection fails (callCount=1), falls through
// Second attempt: createConnection succeeds (callCount=2)
$connection = $pool->pop();
$this->assertSame('x', $connection->getResource());
});
}

public function testPoolEmptyErrorIncludesActiveCount(): void
{
$this->execute(function (): void {
$this->setUpPool(); // size 5
$this->poolObject->setRetryAttempts(1);
$this->poolObject->setRetrySleep(0);

// Pop all 5
$this->poolObject->pop();
$this->poolObject->pop();
$this->poolObject->pop();
$this->poolObject->pop();
$this->poolObject->pop();

try {
$this->poolObject->pop();
$this->fail('Should have thrown');
} catch (Exception $e) {
$this->assertStringContainsString('active 5', $e->getMessage());
}
});
}

public function testUseReclainsConnectionOnCallbackException(): void
{
$this->execute(function (): void {
$this->setUpPool(); // size 5

// use() should reclaim the connection even when callback throws
try {
$this->poolObject->use(function ($resource): void {
$this->assertSame(4, $this->poolObject->count());
throw new \RuntimeException('Callback failed');
});
} catch (\RuntimeException) {
// expected
}

// Connection should be reclaimed, pool back to full
$this->assertSame(5, $this->poolObject->count());
});
}

public function testPoolTelemetry(): void
{
$this->execute(function (): void {
Expand Down
Loading