Skip to content

Release the connection when the statement pool declines to retain a statement - #8

Open
webpatser wants to merge 1 commit into
amphp:2.xfrom
webpatser:fix-statement-pool-connection-release
Open

Release the connection when the statement pool declines to retain a statement#8
webpatser wants to merge 1 commit into
amphp:2.xfrom
webpatser:fix-statement-pool-connection-release

Conversation

@webpatser

Copy link
Copy Markdown

A pool with maxConnections: 1 deadlocks on the second prepare() + execute() while the first (fully consumed) result is still referenced. More generally, N live prepared-and-executed statements starve a pool of N connections.

Repro

Against amphp/mysql v3.1.1 with sql-common v2.0.4 (same behavior on amphp/postgres):

$pool = new MysqlConnectionPool(MysqlConfig::fromString('...'), maxConnections: 1);

$statement = $pool->prepare('SELECT 1');
$result = $statement->execute();

foreach ($result as $row) {
    // fully consumed; $result stays in scope below
}

$second = $pool->prepare('SELECT 2');
$secondResult = $second->execute(); // waits forever

Mechanism

SqlStatementPool::push() guards against retaining statements when the pool is saturated, exactly as its docblock intends. But the decline is a bare return: it neither closes the statement nor drops the reference the release closure in execute() holds. The declined SqlPooledStatement therefore stays alive for as long as the caller references the result object, its connection stays checked out, and SqlCommonConnectionPool::pop() waits on $awaitingConnection, which only that statement's destruction could complete.

Holding a fully consumed result should not pin a connection; that is the same contract #2 established for plain query results.

Fix

  • The decline paths in push() now close the statement, so its connection returns to the pool immediately.
  • The release closure in execute() drops its statement reference after pushing, so the closure itself cannot keep a declined statement (and its connection) alive through a long-lived result object.

The retain path is unchanged; a retained statement behaves exactly as before.

Tests

SqlStatementPoolTest previously stubbed the pool without getConnectionLimit() / getConnectionCount() / getIdleConnectionCount(), so the retention guards always declined with limit 0 and the retention logic was never exercised. Added two cases: a saturated pool must close the declined statement and prepare a fresh one on the next execute (fails before this change), and a pool with capacity must retain and reuse the statement without closing it.

Heads-up for amphp/postgres: with declined statements now being closed, a DEALLOCATE can be sent for a statement the server already dropped via the pool's DISCARD ALL reset. I have a follow-up PR for that ready and will link it here.

@webpatser

Copy link
Copy Markdown
Author

The amphp/postgres follow-up mentioned above is up: amphp/postgres#74. It makes the DEALLOCATE that this change can trigger after a DISCARD ALL reset tolerant instead of wedging the connection.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Development

Successfully merging this pull request may close these issues.

1 participant