Fix for the "next_deadline" bug. #2
Conversation
serges147
commented
Aug 22, 2024
serges147
commented
- 1st commit is just unit test which reproduces the problem
- 2nd commit - the fix, so CI is green again.
| // So, `next_deadline` should be `time_point::max()` b/c there will be nothing left pending after spin. | ||
| SteadyClockMock::advance(1000ms + 30ms); | ||
| const auto out = evl.spin(); | ||
| EXPECT_THAT(out.next_deadline.time_since_epoch(), time_point::max().time_since_epoch()); |
There was a problem hiding this comment.
At this line @ 350 we've got incorrect next deadline time before the fix (1000ms instead of correct max).
| }); | ||
|
|
||
| const auto out = evl.spin(); | ||
| EXPECT_THAT(out.next_deadline.time_since_epoch(), duration::max()); |
There was a problem hiding this comment.
And @ 375 as well.
| .approx_now = time_point::min()}; | ||
| if (tree_.empty()) | ||
| { | ||
| result.approx_now = Clock::now(); |
There was a problem hiding this comment.
In original implementation, we always had 2 consecutive now() calls on the very first loop iteration for the future scheduled event. Now we do only minimum (1) call per iteration.
There was a problem hiding this comment.
Please tag this branch with [[unlikely]]. Maybe there are other places that could use this.
| lateness = now - next; | ||
| if (lateness < duration::zero()) // Nope, even with the latest time sample we are still early -- exit. | ||
| result.approx_now = Clock::now(); // The number of calls to Clock::now() is minimized. | ||
| if (result.approx_now < deadline) // Nope, even with the latest time sample we are still early -- exit. |
There was a problem hiding this comment.
Note, that I also eliminated subtraction (which is over/under-flow prone strictly speaking). Now I subtract later, when I know for sure that now >= deadline (at line @ 333). Just in case, I will add separate unit test with deadline scheduling time near to timepoint::max - probably it will overflow even now...
| EXPECT_THAT(1, evl->getTree().size()); // Second dropped. | ||
| EXPECT_TRUE(a); | ||
| EXPECT_THAT(13'000ms, a.value().deadline.time_since_epoch()); | ||
| EXPECT_THAT(a.value().deadline.time_since_epoch(), 13'000ms); |
There was a problem hiding this comment.
In gtest it is always first actual value, and 2nd-ly the expected - result output (in case of failure) explicitly mentions what was expected and was actually found).
| .approx_now = time_point::min()}; | ||
| if (tree_.empty()) | ||
| { | ||
| result.approx_now = Clock::now(); |
There was a problem hiding this comment.
Please tag this branch with [[unlikely]]. Maybe there are other places that could use this.
