writer: potential deadlocks on cleanup #13
Merged
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
…gnal the waitgroup
When Writer cleans up after a connection error it is possible for there to be a race between the cleanup process (which stops the goroutine responsible for receiving on transactionChan) and new concurrent writes (which send to transactionChan), resulting in potential deadlocks until a new successful connection is made. This is exemplified in nsq_to_nsq, where Writer is used alongside Reader such that, after seeing an error writing, Reader enters backoff mode and sends RDY 1... Because a stray write would be blocked on sending to transactionChan the source nsqd would (correctly) stop sending additional messages (RDY 1 == in-flight 1). This, coupled with the fact that Writer only lazily connects to its destination hosts, meant that until that in-flight message timed out at the source nsqd there would be no additional message delivered to force a re-connect and free up the deadlock. This fix adds accounting for the # of concurrent writers and ensures that transactionCleanup() recvs on transactionChan until that count reaches 0.
|
So changes LGTM. I've taken the liberty of deploying this to production, and will get back to you if this successfully resolves some of the issues we have been seeing. |
|
🔥 🚒 😈 |
|
Rebase/Squash please. |
|
no |
jphines
pushed a commit
that referenced
this pull request
Oct 10, 2013
writer: potential deadlocks on cleanup
|
❤️ @jphines |
sthulb
pushed a commit
to HailoOSS/go-nsq
that referenced
this pull request
Sep 14, 2016
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.
When
Writercleans up after a connection error it is possible for there to be a race between the cleanup process (which stops the goroutine responsible for receiving ontransactionChan) and new concurrent writes (which send totransactionChan), resulting in potential deadlocks until a new successful connection is made.This is particularly problematic for
nsq_to_nsqwhereWriteris used alongsideReader. After seeing an error writing,Readerenters backoff mode and sendsRDY 1. Because a stray write is blocked on sending totransactionChanthe sourcensqdwould (correctly) stop sending additional messages (RDY 1 == in-flight 1).This, coupled with the fact that
Writeronly lazily connects to its destination hosts, means that until the in-flight message times out at the source there would be no additional message delivered to force a re-connect and free up the deadlock.This fix adds accounting for the # of concurrent writers and ensures that
transactionCleanup()recvs ontransactionChanuntil that count reaches0.RFR @jphines @jehiah
I'll take payment in 🍻