Fix synchronizer timer #105
Conversation
johningve
force-pushed
the
fix-synchronizer-timer
branch
from
7cac496 to
b9af063
Compare
The timer used by the synchronizer used to be resettable, but after 7776a29, the closure passed to the timer should only be used once because it stores the current view. This was done so that we can avoid the need for any synchronization of access to the current view in the synchronizer. However, I failed to realize that this would make the timer only usable once. Thus, resetting it is not possible as the view that is stored in the timer's closure would not change upon reset. This commit wraps the timer in a `oneShotTimer` struct that is intended to prevent the timer from being reused. Instead, the methods `startTimeoutTimer` and `stopTimeoutTimer` should be used instead of using the timer directly. These functions ensure that each timer is unique and thus a unique closure is used each time, ensuring that we get the correct view and avoid the need for synchronization code.
johningve
force-pushed
the
fix-synchronizer-timer
branch
from
b9af063 to
36a1b7e
Compare
With the timer changes, the network timer is twins needs to be updated.
Due to local timeout the sychronizer is called stopVoting which the gmock is not expecting, so the test cast is failing. Increased the timeout delay to prevent this.
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## master #105 +/- ##
==========================================
+ Coverage 72.44% 73.03% +0.58%
==========================================
Files 63 63
Lines 6301 6308 +7
==========================================
+ Hits 4565 4607 +42
+ Misses 1428 1399 -29
+ Partials 308 302 -6
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. |
meling
left a comment
meling
left a comment
There was a problem hiding this comment.
This looks good. I added a comment in twins/network.go... but I'm okay to merge it as is. We can always tweak the timeout later if we think it can save us some test execution time.
| consensus.NewVotingMachine(), | ||
| crypto.NewCache(ecdsa.New(), 100), | ||
| synchronizer.New(FixedTimeout(0)), | ||
| synchronizer.New(FixedTimeout(1*time.Millisecond)), |
There was a problem hiding this comment.
We should consider if it would be enough to use e.g., 100 * time.Microsecond to avoid that twins tests take longer time. This should be tested and maybe "measured" since I'm not sure this will have an impact, one way or the other.
There was a problem hiding this comment.
I tried with different values, it looks like 1 millisecond is more stable compared to lower durations.
The timer used by the synchronizer used to be resettable, but after
7776a29, the closure passed to the timer should only be used once
because it stores the current view. This was done so that we can avoid
the need for any synchronization of access to the current view in the
synchronizer. However, I failed to realize that this would make the
timer only usable once. Thus, resetting it is not possible as the view
that is stored in the timer's closure would not change upon reset.
This PR wraps the timer in a
oneShotTimerstruct that is intended toprevent the timer from being reused. Instead, the methods
startTimeoutTimerandstopTimeoutTimershould be used instead ofusing the timer directly. These functions ensure that each timer is
unique and thus a unique closure is used each time, ensuring that we get
the correct view and avoid the need for synchronization code.