-
Notifications
You must be signed in to change notification settings - Fork 915
GODRIVER-3419 Use a semaphore to limit concurrent conn creation #2098
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Open
prestonvasquez
wants to merge
12
commits into
mongodb:master
Choose a base branch
from
prestonvasquez:GODRIVER-3419
base: master
Could not load branches
Branch not found: {{ refName }}
Loading
Could not load tags
Nothing to show
Loading
Are you sure you want to change the base?
Some commits from the old base branch may be removed from the timeline,
and old review comments may become outdated.
+238
−145
Open
Changes from all commits
Commits
Show all changes
12 commits
Select commit
Hold shift + click to select a range
f3d3a2e
GODRIVER-3419 Sketch out pattern
prestonvasquez 58f5fa5
GODRIVER-3419 Mark spawnConnection as TODO
prestonvasquez 7561f45
GODRIVER-3419 Remove tmp file
prestonvasquez b5e51ad
Merge branch 'master' into GODRIVER-3419
prestonvasquez 36f1aff
GODRIVER-3419 Clean up comments
prestonvasquez 8d6fc8e
Merge branch 'GODRIVER-3419' of github.com:prestonvasquez/mongo-go-dr…
prestonvasquez 4b88fd7
Merge branch 'master' into GODRIVER-3419
prestonvasquez 76bf805
queue new connection when removing one to prevent stalled checkOut
prestonvasquez ad100d7
Merge branch 'master' into GODRIVER-3419
prestonvasquez 15b5a47
clarify why we are guarding conn spawn
prestonvasquez e5918e5
Resolve race conflict
prestonvasquez 225249b
Merge branch 'GODRIVER-3419' of github.com:prestonvasquez/mongo-go-dr…
prestonvasquez File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
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
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
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Waiting on the
createConnectionsCond
may cause an unbounded number of goroutines to be paused here.For example, consider a case where the connection pool is full, so
p.hasSpace()
always returnsfalse
, no connections are closing, and all connections are in-use. A new operation callscheckOut
and blocks atp.createConnectionsCond.Wait()
until something signals the cond. It seems that a subsequent call toqueueForNewConn
should signal one of the waiting goroutines, but the behavior ofsync.Cond.Signal
is nondeterministic, so it's not clear if that will always happen.Instead, we could immediately stop trying to create a new connection if the pool is full. The
checkOut
behavior would be the same except if the pool is full, thatcheckOut
would no longer trigger a new connection if another connection is closed while thecheckOut
is waiting.My intuition is that the latter case is lower risk, but it's not clear if goroutines getting "stuck" would even be a problem. What do you think?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@matthewdale I don't think this would work. What happens if we don't await a new connection and the existing connections perish? Then we would be awaiting an idle connection forever:
mongo-go-driver/x/mongo/driver/topology/pool.go
Line 619 in 32c7b85
To prove this you can guard queueForNewConn with hasSpace:
And run the following test:
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@matthewdale Suggest that we guard the goroutine with
p.hasSpace()
but then queue a new connection any time checkInNoEvent is called with a perished connection, ensuring that if checkOut wont indefinitely stall at<-w.ready
.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That sounds like a good idea. Specifically, the current implementation in
checkInNoEvent
that checks if there's awantConn
on thenewConnWait
queue would prevent creating connections when they aren't requested. In that case, we should remove theWait
fromwaitForNewConn
and convertcreateConnectionsCond
to async.Mu
(in fact, we can probably removewaitForNewConn
and in-line the remaining code).