fix: data-race when sending a message from consensus/reactor to a peer via p2p.Channel #334
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
shotonoff
changed the title
shotonoff
force-pushed
the
TD-65-fix-data-race-test-reactor-invalid-block-chain-lock
branch
from
7f89cfe
to
e1db9a9
Compare
lklimek
requested changes
Apr 8, 2022
internal/consensus/reactor.go
Outdated
| default: | ||
| } | ||
| select { | ||
| case <-channel.Done(): |
There was a problem hiding this comment.
Isn't it already handled in https://github.com/dashevo/tenderdash/pull/334/files#diff-9078ec3c9703c58bae5bf3118fce44918746dd40eaa053efaf642dbafbb12e80R101 ? If so, we can remove this condition here.
internal/consensus/reactor.go
Outdated
| @@ -662,31 +665,40 @@ func (r *Reactor) sendCommit(ps *PeerState, commit *types.Commit) error { | |||
| // If to is nil, message will be broadcasted. | |||
| func (r *Reactor) send(ps *PeerState, channel *p2p.Channel, msg proto.Message) error { | |||
| select { | |||
| case <-channel.Done(): | |||
There was a problem hiding this comment.
Isn't it already handled in https://github.com/dashevo/tenderdash/pull/334/files#diff-9078ec3c9703c58bae5bf3118fce44918746dd40eaa053efaf642dbafbb12e80R101 ? If so, we can remove this condition here.
shotonoff
commented
Apr 8, 2022
…ce-test-reactor-invalid-block-chain-lock
…ix-data-race-test-reactor-invalid-block-chain-lock
shotonoff
deleted the
TD-65-fix-data-race-test-reactor-invalid-block-chain-lock
branch
shotonoff
pushed a commit
that referenced
this pull request
Apr 11, 2022
* refactor(consensus): consolidate redundant code * refactor(consensus): improve logging in reactor * refactor(consensus): convert make*Message factory funcs to methods of respective types * fix: data-race when sending a message from consensus/reactor to a peer via p2p.Channel (#334) * fix: add an additional select block to check were a chennel / peer or reactor closed * fix: implement Channel.Send method to safely send messages * fix: add Channel.Send call for broadcasting * fix: remove duplication checking of closed channel * fix: remove first select statement from broadcast method
shotonoff
pushed a commit
that referenced
this pull request
May 10, 2022
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.
Issue being fixed or feature implemented
After some refactoring in
refactor/consensus-reactor-send-broadcastbranch.there was detected data-race between reading data from GO channel and closing it.
What was done?
To solve the data-race issue in
p2p.Channelwas added an additionalselectblock to check, were channel, a peer, or a reactor close to stopping sending a messageHow Has This Been Tested?
Unit-test
Breaking Changes
No
Checklist:
For repository code-owners and collaborators only