Fixes #1165 by having threads wait for any outstanding connect to finish. #1170
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
|
Also noticed that we didn't bump the ref count in the race condition case, so those might have behaved strangely in some rare cases. Pushed a fix for that. |
slackpad
force-pushed
the
b-connection-spam
branch
from
49a3022
to
5524b95
Compare
slackpad
force-pushed
the
b-connection-spam
branch
from
5524b95
to
8bca3eb
Compare
|
That last change makes it so only one thread attempts the connect and everybody else goes based on that. I'll let that bake for a little while (it's more tricky, imho, but not too bad). |
| }() | ||
|
|
||
| c, err := p.getNewConn(dc, addr, version) | ||
| if err != nil { |
There was a problem hiding this comment.
Super minor, but can we move the code in the defer to after getNewConn and delete the limiter entry and add the pool entry inside the same critical section? Just to save a defer and another round of locking
| var wait chan struct{} | ||
| var ok bool | ||
| if wait, ok = p.limiter[addr.String()]; !ok { | ||
| wait = make(chan struct{}, 1) |
There was a problem hiding this comment.
We probably don't need to buffer it, since we just close
|
LGTM! |
slackpad
added a commit
that referenced
this pull request
Aug 13, 2015
Fixes #1165 by having threads wait for any outstanding connect to finish.
slackpad
added a commit
that referenced
this pull request
Aug 13, 2015
Fixes #1165 by having threads wait for any outstanding connect to finish.
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.
This creates a map of channels (per-address) that throttles requests for connections to one at a time (per-address). The other threads will then quickly grab the newly-created connection one at a time and use it.
If the connection they were waiting for fails, they will all try to make a new connection, and the race condition logic for that case was preserved. This seems like the best thing to do in order to move forward, since they all could have been hanging around waiting for that one connection to go. They essentially revert to the behavior we have today, but only in the case where there's network trouble.
I was trying to find a simpler way to handle the case where the connection they are waiting on fails - we could fail all the others immediately but I think the code would be a little more complicated. Thundering a herd at a dead guy seems pretty low-impact.