Skip to content

Commit f8b9a7f

Browse files
authored
Fix wasm and time computations (#164)
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
1 parent bc99406 commit f8b9a7f

File tree

1 file changed

+20
-13
lines changed
  • quinn-proto/src/connection

1 file changed

+20
-13
lines changed

quinn-proto/src/connection/mod.rs

Lines changed: 20 additions & 13 deletions
Original file line numberDiff line numberDiff line change
@@ -2275,8 +2275,7 @@ impl Connection {
22752275
Duration::from_micros(ack.delay << self.peer_params.ack_delay_exponent.0),
22762276
)
22772277
};
2278-
let rtt = instant_saturating_sub(
2279-
now,
2278+
let rtt = now.saturating_duration_since(
22802279
self.spaces[space].for_path(path).largest_acked_packet_sent,
22812280
);
22822281

@@ -2495,8 +2494,6 @@ impl Connection {
24952494
.max(TIMER_GRANULARITY);
24962495
let first_packet_after_rtt_sample = path.first_packet_after_rtt_sample;
24972496

2498-
// Packets sent before this time are deemed lost.
2499-
let lost_send_time = now.checked_sub(loss_delay).unwrap();
25002497
let largest_acked_packet = self.spaces[pn_space]
25012498
.for_path(path_id)
25022499
.largest_acked_packet
@@ -2518,8 +2515,11 @@ impl Connection {
25182515
persistent_congestion_start = None;
25192516
}
25202517

2521-
if info.time_sent <= lost_send_time || largest_acked_packet >= packet + packet_threshold
2522-
{
2518+
// Packets sent before now - loss_delay are deemed lost.
2519+
// However, we avoid substraction as it can panic and there's no
2520+
// saturating equivalent of this substraction operation with a Duration.
2521+
let packet_too_old = now.saturating_duration_since(info.time_sent) >= loss_delay;
2522+
if packet_too_old || largest_acked_packet >= packet + packet_threshold {
25232523
// The packet should be declared lost.
25242524
if Some(packet) == in_flight_mtu_probe {
25252525
// Lost MTU probes are not included in `lost_packets`, because they
@@ -2564,7 +2564,7 @@ impl Connection {
25642564
now,
25652565
lost_packets,
25662566
lost_mtu_probe,
2567-
lost_send_time,
2567+
loss_delay,
25682568
in_persistent_congestion,
25692569
size_of_lost_packets,
25702570
);
@@ -2600,7 +2600,7 @@ impl Connection {
26002600
now,
26012601
lost_pns,
26022602
in_flight_mtu_probe,
2603-
now,
2603+
Duration::ZERO,
26042604
false,
26052605
size_of_lost_packets,
26062606
);
@@ -2625,7 +2625,7 @@ impl Connection {
26252625
now: Instant,
26262626
lost_packets: Vec<u64>,
26272627
lost_mtu_probe: Option<u64>,
2628-
lost_send_time: Instant,
2628+
loss_delay: Duration,
26292629
in_persistent_congestion: bool,
26302630
size_of_lost_packets: u64,
26312631
) {
@@ -2652,6 +2652,17 @@ impl Connection {
26522652
"packets lost",
26532653
);
26542654

2655+
// Packets sent before this time are deemed lost.
2656+
// We avoid computing this value above, since it's possible for this to panic
2657+
// if the `loss_delay` value internally stores a bigger `Duration` than the
2658+
// `Duration` that's stored inside the `Instant`, because some platforms may
2659+
// implement the `Instant` with a counter relative to system or even process
2660+
// startup (Wasm is one such case).
2661+
// If we're at this point, then it must be possible to have instants that are
2662+
// longer ago than `loss_delay` (see the `packet_too_old` computation
2663+
// above).
2664+
let lost_send_time = now.checked_sub(loss_delay).unwrap();
2665+
26552666
for &packet in &lost_packets {
26562667
let Some(info) = self.spaces[pn_space].for_path(path_id).take(packet) else {
26572668
continue;
@@ -5915,10 +5926,6 @@ impl From<PathEvent> for Event {
59155926
}
59165927
}
59175928

5918-
fn instant_saturating_sub(x: Instant, y: Instant) -> Duration {
5919-
if x > y { x - y } else { Duration::ZERO }
5920-
}
5921-
59225929
fn get_max_ack_delay(params: &TransportParameters) -> Duration {
59235930
Duration::from_micros(params.max_ack_delay.0 * 1000)
59245931
}

0 commit comments

Comments
 (0)