[6.0.1] Use read-write lock to protect concurrent pool accesses (#26658) #26697
Conversation
This file contains 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
AndriySvyryd
approved these changes
Nov 15, 2021
AndriySvyryd
changed the title
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.
Resubmit of #26658 which was accidentally merged before tactics approval and reverted in #26696. This is identical to #26658 and has already been approved by the team.
As this is being considered for servicing, I went with the least invasive/risky option, and introduced read/write locking. Using a ConcurrentDictionary is definitely possible, but would require other changes - specifically in how pruning is implemented. We can clean this up and improve for 7.0. I couldn't repro this in some quick testing efforts (tricky concurrency issue), if we think it's really worth it I can spend more time on it.
Fixes #26612
Description
Locking is missing when looking up a pool group in a Dictionary in SQLite's pooling implementation.
Customer impact
If code happens to look up a pool group at the same time as a modification of the pool group dictionary is happening (e.g. opening a connection with a new connection string), an exception may be thrown (or in theory, possibly undefined behavior).
How found
Reported by @sebastienros
Regression
No, SQLite connection pooling is new in 6.0.
Testing
Testing this is somewhat tricky, and would not be completely reliable as this is a concurrency condition relying on a race condition.
Risk
Low, the current (partial) locking scheme has been augmented with read-write locking, changes are very localized. Added a quirk mode.