clocks: tagged deadlines

[yossigi]

Summary

"I needed the clocks change to fix an issue with dynamic lambda (currently, if there are two deadlines for the same time, only one of them fires). This is a potential issue for dynamic lambda (if accidentally, the filter deadline matches a different deadline) and also made it very hard to test (since the current test suite allowed triggering a deadline by their time, which I don’t know apriori, rather than by their type)."

Test Plan

Existing tests + a few simulate cases confirming identical timeouts do not conflict.

[zeldovich]

I think it would be reasonable to require there's exactly one timeout of a given type, which would allow you to avoid the double-nested-map in the implementations of Monotonic, testingClock, etc, and just have a single-level map with TimeoutType as the key.

[zeldovich]

Consider making TimeoutType a generic type argument in timers.Clock to avoid hard-coding the three types of agreement timeouts in the util/timers package, as in:

zeldovich@b59136b

[codecov]

Codecov Report

Merging #5649 (dfac007) into master (4ff2bf3) will decrease coverage by 1.38%.
Report is 14 commits behind head on master.
The diff coverage is 74.57%.

@@            Coverage Diff             @@
##           master    #5649      +/-   ##
==========================================
- Coverage   55.13%   53.75%   -1.38%     
==========================================
  Files         465      465              
  Lines       65022    65027       +5     
==========================================
- Hits        35852    34958     -894     
- Misses      26780    27644     +864     
- Partials     2390     2425      +35     

Files Changed	Coverage Δ
agreement/types.go	78.46% <ø> (-9.24%)	⬇️
node/node.go	3.59% <0.00%> (-0.56%)	⬇️
util/timers/frozen.go	0.00% <0.00%> (ø)
util/timers/monotonic.go	84.21% <90.47%> (+5.83%)	⬆️
agreement/agreementtest/simulate.go	86.66% <100.00%> (ø)
agreement/demux.go	91.20% <100.00%> (ø)
agreement/persistence.go	77.61% <100.00%> (+0.33%)	⬆️
agreement/player.go	95.94% <100.00%> (-0.28%)	⬇️
agreement/service.go	86.11% <100.00%> (+0.12%)	⬆️

... and 48 files with indirect coverage changes

📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more

[yossigi]

I think it would be reasonable to require there's exactly one timeout of a given type, which would allow you to avoid the double-nested-map in the implementations of Monotonic, testingClock, etc, and just have a single-level map with TimeoutType as the key.

That would change the current behavior though. Since now you're allowed to specify multiple deadlines (of any kind). And, the clock remembers past deadlines, so they expire immediately the next time someone looks for them, but without remembering that past deadline, that behavior would also change.
Looking through the code though, I don't think it would break anything; though it's still a subtle change from current behavior.

[zeldovich]

That would change the current behavior though. Since now you're allowed to specify multiple deadlines (of any kind). And, the clock remembers past deadlines, so they expire immediately the next time someone looks for them, but without remembering that past deadline, that behavior would also change.

This part of the behavior can easily stay the same: calling TimeoutAt() with the same arguments would return the same channel, like it does now.

One other benefit is that it codifies the expected usage pattern: that there's just one timeout of a given type. All current code follows this (as far as I can tell), and this makes testing easier (you can trigger a timeout by naming it with the type, rather than by specifying timeout durations). Supporting the more-complicated usage pattern of multiple timeouts per type makes it harder to write correct test harnesses, without any concrete benefit (since no callers need this behavior).

[gmalouf]

Current implementation aligns with problem Yossi is trying to solve with new cases exercising, works for me.

[cce]

@zeldovich for your generic implementation would you need to call Clock.Zero separately for each of the types in rezeroAction?

[cce]

Oh I think I understand better, they all still share a Zero() and still have a map[TimeoutType] but you're just moving the enum def out of the timers package using generics..

[zeldovich]

Yeah, that's all that's going on. It seemed a little weird to hard-code three types of timeouts in util/timers.

[algorandskiy]

IMO using TimeoutType that basically an enum with 3 values as generic is a bit strange to me but I can live with it.