Fix crash when Instant::now() returns the same value twice #830
Conversation
Lorak-mmk
force-pushed
the
fix_latency_crash
branch
from
7299df7 to
e572061
Compare
There was a problem hiding this comment.
Looks good, thanks. Only one thing - please swap the commits together. I know it could make some sense to introduce a failing test first and then fix it in order to better show that the fix works (e.g. I checked out the branch and saw how it behaves with and without the fix as a part of the review) - but this breaks bisectability.
Lorak-mmk
force-pushed
the
fix_latency_crash
branch
from
e572061 to
cae3f03
Compare
|
Done |
| @@ -2192,13 +2192,37 @@ mod latency_awareness { | |||
| Some(prev_avg) => Some({ | |||
| let delay = (now - prev_avg.timestamp).as_secs_f64(); | |||
There was a problem hiding this comment.
https://doc.rust-lang.org/std/time/struct.Instant.html#monotonicity
Assuming that compute_next could be called with timestamp that is smaller than prev_avg.timestamp, this code will... work correctly as of now, but may start to panic in the future. Let's use checked_duration_since here to be sure.
There was a problem hiding this comment.
Is the current version ok?
There was a problem hiding this comment.
Yes, it's OK. TBF I realized that saturating_duration_since would be nicer and shorter, but I won't insist on changing that (unless clippy complains)
There was a problem hiding this comment.
Changed it, just to be safe in the future
Lorak-mmk
force-pushed
the
fix_latency_crash
branch
from
cae3f03 to
2586f29
Compare
When Instant::now() returns the same value during 2 calls of TimestampedAverage::compute_next, division by 0 occurs, and Duration::from_secs_f64 panics because of NaN input. This commit fixes the issue and adds additional logging in case another conversion problem occurs in the future.
When Instant::now() returns the same value during 2 calls of TimestampedAverage::compute_next, division by 0 occurs, and Duration::from_secs_f64 panics because of NaN input. This commit adds regression test for this problem.
Lorak-mmk
force-pushed
the
fix_latency_crash
branch
from
2586f29 to
e900ac5
Compare
When Instant::now() returns the same value during 2 calls of
TimestampedAverage::compute_next, division by 0 occurs,
and Duration::from_secs_f64 panics because of NaN input.
This PR fixes the issue, adds additional logs in case another conversion problem
occurs in this code and adds regression test for the issue.
Pre-review checklist
./docs/source/.Fixes:annotations to PR description.