fix: Guarantee event processing order #1352
Conversation
AndrewSisley
added
bug
This fixes #1349 where events are attempted to be handled after database close
AndrewSisley
force-pushed
the
sisley/fix/I1351-event-order-of-ops
branch
from
2673ea4
to
5e69288
Compare
Codecov Report
@@ Coverage Diff @@
## develop #1352 +/- ##
===========================================
- Coverage 70.66% 70.62% -0.05%
===========================================
Files 184 184
Lines 17819 17818 -1
===========================================
- Hits 12592 12584 -8
- Misses 4275 4279 +4
- Partials 952 955 +3
|
| } | ||
|
|
||
| // NewSimpleChannel creates a new simpleChannel with the given subscriberBufferSize and | ||
| type subscribeCommand[T any] Subscription[T] |
There was a problem hiding this comment.
Note: Fred suggested dropping the command suffix for these, or shortening it to cmd. I have a minor preference for the current names (I like it being clear that they are commands, and not an e.g. subscribe type, I also prefer the more verbose name).
Happy to change this though if others want, or if Fred's preference is/becomes stronger than I think it is :)
There was a problem hiding this comment.
I would be against dropping the suffix because at the moment it clearly shows that these types belong to a single concept. Shortening would be fine though especially because cmd is well-known. But my personal preference is to keep command
There was a problem hiding this comment.
Prefer to keep it the way it is.
There was a problem hiding this comment.
Will keep as-is then :)
Language rules prevented the same for publishCommand as the generic is not allowed (would be ).
This is a change from the prior solutino, as the (un)subscribe and event channels were separate.
AndrewSisley
force-pushed
the
sisley/fix/I1351-event-order-of-ops
branch
from
e169c05
to
55b61c7
Compare
Really glad to hear - thanks again for your help with this one :) |
* Guarantee event processing order * Make Close synchronous This fixes sourcenetwork#1349 where events are attempted to be handled after database close
Relevant issue(s)
Resolves #1351 #1349
May also resolve much of the current test flakiness, but there may be other underlying issues so I'll leave those open for a bit. It does not resolve all of the flakiness and I have seen one P2P local failure on this branch (logged in #1338 )
Description
Modifies the simpleChannel to guarantee the processing order of stuff.
The previous solution did not do this, and allowed new subscriptions to pick up database events that had previously occurred but had not yet been processed by
handleChannel.A common scenario for how this can result in a test failure is documented in #1351
Also includes a fix for #1349 as I believe it occurs in the same part of the codebase, for similar reasons, and there is no point putting up a second, dependant review for it.