ConnectionPool: release the claimed slot when the factory throws - #46
Conversation
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) <noreply@anthropic.com>
|
Important Review skippedAuto reviews are disabled on base/target branches other than the default branch. Please check the settings in the CodeRabbit UI or the ⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 9fa5bf23f0
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| // 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); |
There was a problem hiding this comment.
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 👍 / 👎.
There was a problem hiding this comment.
Pull request overview
This PR fixes a slot-leak bug in Semitexa\Orm\Adapter\ConnectionPool where a pool slot was claimed (via Atomic::cmpset) before invoking the connection factory, and a thrown exception would previously leave the created counter permanently incremented—eventually wedging the pool into a “full-but-empty” state that can block indefinitely.
Changes:
- Roll back the claimed
createdslot inConnectionPool::pop()when the factory throws, then rethrow the exception. - Apply the same rollback behavior in
ConnectionPool::fill()when the factory throws. - Add a PHPUnit test that repeatedly simulates factory failure and asserts
creatednever leaks and the pool remains usable after recovery.
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated no comments.
| File | Description |
|---|---|
| tests/Unit/Adapter/ConnectionPoolTest.php | Adds regression coverage ensuring factory exceptions don’t leak pool slots and the pool recovers. |
| src/Adapter/ConnectionPool.php | Adds try/catch rollback logic to prevent created counter leaks in pop() and fill(). |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Problem
ConnectionPool::pop()(andfill()) claim a pool slot with an atomiccmpsetbefore calling the connection factory. The factory is a realnew PDO(...)connect, which throws on any failure — a transient network blip, MySQL momentarily refusing, an auth hiccup. When it threw, the exception propagated out with the slot still counted: nothing anywhere decrementscreatedexcept full teardown.So every failed connect permanently burned a slot. After
sizesuch failures the pool is full-but-empty forever — the fast-path guardisEmpty() && current < sizeis never true again, so everypop()falls through toChannel->pop($timeout)with the defaulttimeout = -1and blocks its caller indefinitely, waiting for a connection that will never be pushed. A ratcheting deadlock that only a process restart cleared.Production impact
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. MySQL itself was healthy and nowhere near its connection limit (Max_used_connections88 / 151) — because the "held" connections were phantom slots that were never actually opened. That mismatch (app deadlocked, DB fine) is the signature.Fix
Wrap the factory call in
try/catchand roll back thecmpsetwithcreated->sub(1)before rethrowing, in bothpop()andfill(). A failed connect now leaves the slot free for the next attempt, so transient DB unavailability degrades gracefully (the request errors) instead of permanently poisoning the pool.Test
Adds
pop_releases_the_claimed_slot_when_the_factory_throws: drives repeated factory failures and asserts the slot counter never leaks and the pool stays usable once the factory recovers. It fails fast (assertscreated == 0before any blockingpop()) rather than hanging against the unpatched code — verified: it errors with "Failed asserting that 1 is identical to 0" on the old code and passes on the fix.🤖 Generated with Claude Code