Fix race conditions in the connection pool code. #18447
Open
Conversation
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
Signed-off-by: Arthur Schreiber <[email protected]>
Review ChecklistHello reviewers! 👋 Please follow this checklist when reviewing this Pull Request. General
Tests
Documentation
New flags
If a workflow is added or modified:
Backward compatibility
|
vitess-bot
bot
added
NeedsBackportReason
|
Unfortunately, this does have performance impact on the pool performance (but whether that's noticeable in real world scenarios is unclear to me).
|
arthurschreiber
added
Backport to: release-20.0
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #18447 +/- ##
==========================================
- Coverage 67.51% 67.48% -0.03%
==========================================
Files 1607 1607
Lines 262687 262854 +167
==========================================
+ Hits 177340 177374 +34
- Misses 85347 85480 +133 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
|
I have a test over at https://gist.github.com/arthurschreiber/fffe5c2ecf550e75fdee6e97cc08f9df that can reproduce the problem. Unfortunately, the test is flaky so I've not added it to this PR. |
dbussink
reviewed
Jul 17, 2025
| } else { | ||
| stack := connSetting.bucket & stackMask | ||
| pool.settings[stack].Push(conn) | ||
| pool.freshSettingsStack.Store(int64(stack)) |
There was a problem hiding this comment.
I wonder, would an alternative approach work to avoid having to lock? What if we still optimistically put it into the stack, but then check after we've completed that if we have since been closed and then remove it again?
So you only pay the price during closing mostly and then having to do any locking or other synchronization?
There was a problem hiding this comment.
I'm definitely open to alternative approaches. For your suggestion, don't we have to take a lock on capacityMu to correctly decide whether the pool is closed or not?
There was a problem hiding this comment.
We can think about optimistic concurrency approach with epoch validation.
ConnPool can have epoch atomic.Uint64. We invalidated with increment whenever there is a change like Close or Capacity.
In tryReturnConn we read the epoch value and then read the capacity. Before returning the connection back to the stack we check the epoch again If it is changed we continue otherwise return to the stack.
After returning we check again and if there is a change we try to get the connection back, if we receive the connection continue again.
We do not expect capacity to change that frequently.
There was a problem hiding this comment.
I am working on an additional change that uses a pool generation number to allow Close() to return instantly and basically break the link between checked out connections and the pool.
I think that could be used for the optimistic concurrency check. I'll try this out and see if I can make this work.
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.
Description
This fixes the two race conditions described in #18202 (comment)
Related Issue(s)
#18202
Checklist
Deployment Notes