Release pooled connections when their borrowing coroutine exits - #14
Merged
Merged
Conversation
Worker::handle() only tears down the HTTP request coroutine's borrows. Connections borrowed by child coroutines and coroutine-based daemons bypassed release() entirely: the pool counter drifted one slot per borrow toward false 'Connection pool exhausted', and long-lived borrowers accumulated leaked server-side prepared statements that max_lifetime recycling could never reach (recycling happens at release). Two mechanisms, defense in depth: - DatabaseManager arms a Coroutine::defer at borrow time that releases whatever the coroutine still holds when it ends. A no-op for the request path (the Worker empties the context first). - DatabasePool tracks every connection it created via WeakReference and heals the counter for any that were garbage-collected without release() - covering paths the defer cannot see. poolStats() now also reports tracked_connections and long_borrowed_connections (borrows held past max_lifetime - a live pointer at daemons that never release).
- Heal a dead tracked slot when its object id is reused by a new connection: allocation-triggered gc inside factory->make() can free an abandoned connection and hand its id to the connection being created, and overwriting the dead weak reference would strand that slot's counter increment forever - reopening the drift-to-exhaustion class. - Collect the coroutine's cycle garbage in the exit hook before releasing, mirroring the Worker's safe-point ordering, so statements hidden in cycles close against a still-idle connection. - Skip the all-pool prune when the exit hook's release walk finds an empty context - the request path already released and pruned through the Worker, and paid a redundant full channel drain per request. - Reconcile in getStats() so a quiet pool does not report vanished borrowers as live or long-borrowed. - Document the bounded double-defer after Context::clear() and the explicit-release pattern for daemon loops.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Problem
Worker::handle()only tears down the HTTP request coroutine's pooled connections. Connections borrowed by child coroutines (Coroutine::createin app code) and coroutine-based daemons bypassedrelease()entirely, with two consequences:max_lifetimerecycling only runs at release, so a connection that never releases accumulates leaked server-side prepared statements indefinitely (the error-1461 incident class).Fix — defense in depth
DatabaseManagerarms aCoroutine::deferat borrow time (once per coroutine): at coroutine exit it collects the coroutine's cycle garbage at the safe point (same ordering as the Worker) and releases whatever the context still holds. A no-op for request coroutines — the Worker empties the context first.DatabasePooltracks every connection it created viaWeakReferenceand heals the counter for any that were garbage-collected without release — including the object-id-reuse race where an abandoned connection dies insidefactory->make()and the new connection inherits its id.poolStats()now reportstracked_connectionsandlong_borrowed_connections(borrows held past max_lifetime — a live pointer at daemons that never release).Verification
octane:start, real MySQL, this branch): 50 child-coroutine borrows → pool at 1 connection fully returned, 20/20 requests OK, 0 leaked statements (main: exhausted at 10, service down).Known residual
A daemon coroutine that literally never ends still holds its connection (nothing can force-release a borrow in use);
long_borrowed_connectionsnow makes those visible, and such loops should callreleaseConnections()per iteration — documented in the code.Need help on this PR? Tag
@codesmith-botwith what you need. Autofix is disabled.