[pubsub] Prioritise internal subscribers (e.g. reactor) over external (e.g. RPC)#1574
[pubsub] Prioritise internal subscribers (e.g. reactor) over external (e.g. RPC)#1574
Conversation
consensus/reactor.go
Outdated
| fastSync: fastSync, | ||
| } | ||
| // XXX: modifing state to send us new round steps, votes and proposal heartbeats | ||
| consensusState.reactorChs = &reactorChs{ |
There was a problem hiding this comment.
this is not good, is it? should we bring events pkg to avoid doing this?
|
I had to alter events package - see 30b5688 |
ecc94bc to
9c1a022
Compare
Codecov Report
@@ Coverage Diff @@
## develop #1574 +/- ##
===========================================
+ Coverage 63.57% 64.22% +0.64%
===========================================
Files 103 109 +6
Lines 9538 10633 +1095
===========================================
+ Hits 6064 6829 +765
- Misses 2907 3188 +281
- Partials 567 616 +49
|
5d2cfa8 to
07ea687
Compare
What required this ? |
| // newStep is called by updateToStep in NewConsensusState before the eventBus is set! | ||
| if cs.eventBus != nil { | ||
| cs.eventBus.PublishEventNewRoundStep(rs) | ||
| cs.evsw.FireEvent(types.EventNewRoundStep, &cs.RoundState) |
There was a problem hiding this comment.
why send a shallow copy if I can send an original object? (events is sync. so there is no race)
Not sure how it was done before, but the problem is in order to catch first NewRoundStep I need to subscribe to such events before SwitchToConsensus emits it (30b5688#diff-f50d554d9c7f92cf7686b247b8c91419L85). See 30b5688 Because of that, I can't use |
I had to alter events package for that. Hope that's fine. Refs #847
c0b4fda to
a9d0adb
Compare
|
Sounds good. Test failure: I can reproduce locally on this branch but not on develop. What's that about !? |
|
Oh the message is wrong - it actually expects everyone to be fully connected. And now I can't reproduce it ... |
|
Hmm I reran the test on Circle and it timedout of the consensus test. Ran the consensus tests locally and it failed on Looks like this might be introducing some non-determinism ? |
|
Hmm.. I will take a look tmrw |
maybe, but how? all that got changed is the way CS reactor gets its events. |
|
Yeh especially given the test failure in p2p. Maybe the change is just helping surface previously lurking non-determinism. We're also seeing it in #1607 |
) Bumps [golang.org/x/sync](https://github.com/golang/sync) from 0.4.0 to 0.5.0. - [Commits](golang/sync@v0.4.0...v0.5.0) --- updated-dependencies: - dependency-name: golang.org/x/sync dependency-type: direct:production update-type: version-update:semver-minor ... Signed-off-by: dependabot[bot] <support@github.com> Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com>
Refs #847