fix race conditions in conn.go #134
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
- closeCount renamed to closing; acts as atomic bool
- once replaced with atomic.CompareAndSwapUint32 in setClosing method
- wgSender removed:
- Close could call wgSender.Wait while another goroutine is about to
call wgSender.Add; this is detected by the race detector as a race
condition if Wait hasn't yet returned. the WaitGroup documentation
also lists this scenario as an error.
- the intent appears to be to let all messages queue in the buffered
chan 'chanMessage' before sending MessageQuit. the WaitGroup could
reach 0 and be incremented again by a goroutine which successfully
passed isClosing, therefore the WaitGroup isn't a strong enough
guarantee. this is addressed in the next commit.
|
Nit on defer unlock. Lgtm otherwise, thanks |
- in order to guarantee the Op: MessageQuit is the last in chanMessage we need to lock messageMutex (or any mutex) before setting 'closing' in the Close method and before checking it in the sendProcessMessage method. - there may be other ways to do this, such as sending the close message to a different channel and then draining the chanMessage channel. this would require more accounting work
- remove l.chanMessageID != nil check; setting the chan to nil was removed in 3bda2b4 - change chanConfirm to chan struct{}; remove send, just close - log.Print -> log.Println - since the intention is to close chanMessage after calling Close and receiving on chanConfirm (in the defer of processMessages) the channel should not be closed in the for/select loop
judwhite
force-pushed
the
feature/fix-race
branch
from
adeef92 to
3ca927c
Compare
|
@liggitt Done. |
liggitt
added a commit
that referenced
this pull request
Nov 23, 2017
fix race conditions in conn.go
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.
Commits are separated for easier review. Commit messages have details on the thought process.
I just came across this package tonight, so I'm unfamiliar with its use in real scenarios. I may also be unfamiliar with nuances in the code. If someone could try this in their setup or recommend which use cases need tests I'll happily add them.
/cc @johnweldon @liggitt