-
-
Notifications
You must be signed in to change notification settings - Fork 497
fix(quinn-proto): Avoid underflow panic in packet loss Instant calculations
#2436
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
fix(quinn-proto): Avoid underflow panic in packet loss Instant calculations
#2436
Conversation
3f73e13 to
cec9619
Compare
|
I messed up the computation ever so slightly with a |
c53e8d6 to
592e56a
Compare
Ported from quinn-rs#2436 * Remove `instant_saturating_sub` fn in favor of `Instant::saturating_duration_since` * Avoid underflow panic in packet loss `Instant` calculations
9e329ee to
815f263
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for the iterations!
@Ralith want to have a quick look?
Ralith
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nice, thanks!
Addresses the failure in #2434
Subtractions from
Instantcan underflow (and thus panic), because theu64stored internally might be relative to some startup time that might be too recent.This is what caused the failure in Wasm: The instant was relative to the process startup time, in one case ~15s, but the
loss_delaywas set to ~17s in that case.Instead, this PR aims to compare the saturated difference of
nowandinfo.time_sent, and see if that's bigger thanloss_delayor not.Unfortunately I don't know of a different way of resolving this. There's no such thing as
Instant::saturating_sub, and there's noInstant::MINor similar with which it could be implemented.Even though this addresses a problem in Wasm, this case can also happen in MacOS and other platforms, but requires different, less common edge-cases to trigger.