[FIXED] Ensure state changes are posted in order they occur #12
Conversation
| @@ -36,6 +36,10 @@ type Node struct { | |||
| // Async handler | |||
| handler Handler | |||
|
|
|||
| // Used to chain go routines posting events to async handler. | |||
| nextSCCh chan bool | |||
There was a problem hiding this comment.
Sure. I was trying to keep it small. It standed for next State Change Channel
There was a problem hiding this comment.
Took me awhile to figure that out.. ;) That is why I suggested something more descriptive but still short. Maybe just stateChg?
| @@ -514,6 +520,20 @@ func (n *Node) switchToCandidate() { | |||
| n.switchState(CANDIDATE) | |||
| } | |||
|
|
|||
| // Execute `f` in a separate go routine, but ensures that functions | |||
| // are executed in order. | |||
| func (n *Node) serializeGoRoutine(nextCh *(chan bool), f func()) { | |||
There was a problem hiding this comment.
So this can only handle two channels at most correct? Might want to document if that is the case.
There was a problem hiding this comment.
Should we make it generic where their is a list of channels you can select from?
There was a problem hiding this comment.
No, because the node creates a nextChannel every time, and pass it as a closure to the function.
There was a problem hiding this comment.
So we have N Go routines they are just chained together correct? Feels like we should be able to do with 1 Go routine correct?
There was a problem hiding this comment.
The goal is to not block the node when posting a state change (or error). The default StateChange handler is channel based, and unbuffered (making it buffered would not be a solution anyway). If the user is not dequeuing, the node could be blocked.
Options are for the node to have a single go routine but use an array or list of events that are added and notify the central go routine doing the dispatching.
I feel that in this case, the proposed solution is elegant and simpler. I don't think we can expect thousands of changes a second ;-). But we do want changes to be added to the handler in order. What do you think?
There was a problem hiding this comment.
I hear you, just concerned that we have N Go routines that are lock stepped instead of one that is selecting from multiple channels. But your solution may be better.
There was a problem hiding this comment.
Let me experiment. I will let you know when ready for review, ok?
|
There are 2 commits, 1 for a fix to first implementation (and rename variables). The second is using a single go-routine. I will let you decide which one you prefer. |
|
ok so I merged the other, but should I merge this one? Which one is this? |
|
This one is for the state change ordering issue. Originally go routines would be started and chained together. You were concerned about the number. The current code shows how we could do with a single go routine (and array of functions). |
|
Which do you prefer? |
|
I personally prefer the first one. Simpler and I believe perfectly fine for the use case (don't think there would be too many events). |
|
Regardless, choose and let me know. I will then need to rebase master and do some cleanup (squash/force commits). |
|
Let's go with your suggestion.. thx.. |
Both state changes and errors were posted through the use of a go routine. This did not guarantee that the changes would arrive in the state change handler in the same order they were produced. Resolves #11
kozlovic
force-pushed
the
fix_state_change
branch
from
11b9137
to
03e72c9
Compare
|
Ok, good to go here. |
This is another approach for solving issue #11 that was addressed with PR #12. The advantage of this approach over #12 is that there will be atmost 1 go-routine per type of event (StateChange and error). Added a test that demonstrates that not receiving from state change and error channels do not block the Node. Also fixed a flapping test. Resolves #11
Both state changes and errors were posted through the use of a go
routine. This did not guarantee that the changes would arrive in
the state change handler in the same order they were produced.
Resolves #11