From 86ea03ec86d0c99c4026132af97f71e38eabef17 Mon Sep 17 00:00:00 2001 From: Benjamin Saunders Date: Sat, 2 Sep 2023 13:40:36 -0700 Subject: [PATCH] Push max ACK delay clamping down into ACK frequency logic --- quinn-proto/src/connection/ack_frequency.rs | 13 +++++++++++-- quinn-proto/src/connection/mod.rs | 14 +++++--------- 2 files changed, 16 insertions(+), 11 deletions(-) diff --git a/quinn-proto/src/connection/ack_frequency.rs b/quinn-proto/src/connection/ack_frequency.rs index 7e46ea77e..9f723d95d 100644 --- a/quinn-proto/src/connection/ack_frequency.rs +++ b/quinn-proto/src/connection/ack_frequency.rs @@ -33,9 +33,18 @@ impl AckFrequencyState { /// Returns the `max_ack_delay` that should be requested of the peer when sending an /// ACK_FREQUENCY frame - pub(super) fn candidate_max_ack_delay(&self, config: &AckFrequencyConfig) -> Duration { + pub(super) fn candidate_max_ack_delay( + &self, + config: &AckFrequencyConfig, + peer_params: &TransportParameters, + ) -> Duration { // Use the peer's max_ack_delay if no custom max_ack_delay was provided in the config - config.max_ack_delay.unwrap_or(self.peer_max_ack_delay) + config + .max_ack_delay + .unwrap_or(self.peer_max_ack_delay) + .max(Duration::from_micros( + peer_params.min_ack_delay.map_or(0, |x| x.into()), + )) } /// Returns the `max_ack_delay` for the purposes of calculating the PTO diff --git a/quinn-proto/src/connection/mod.rs b/quinn-proto/src/connection/mod.rs index 4d0805903..cd27a7b5f 100644 --- a/quinn-proto/src/connection/mod.rs +++ b/quinn-proto/src/connection/mod.rs @@ -2988,24 +2988,20 @@ impl Connection { if mem::replace(&mut space.pending.ack_frequency, false) { let sequence_number = self.ack_frequency.next_sequence_number(); - // Safe to unwrap because these are always provided when ACK frequency is enabled + // Safe to unwrap because this is always provided when ACK frequency is enabled let config = self.config.ack_frequency_config.as_ref().unwrap(); - let min_ack_delay_micros = self.peer_params.min_ack_delay.unwrap(); // Ensure the delay is within bounds to avoid a PROTOCOL_VIOLATION error - let max_ack_delay = self.ack_frequency.candidate_max_ack_delay(config); - let max_ack_delay_micros = max_ack_delay - .as_micros() - .try_into() - .unwrap_or(VarInt::MAX) - .max(min_ack_delay_micros); + let max_ack_delay = self + .ack_frequency + .candidate_max_ack_delay(config, &self.peer_params); trace!(?max_ack_delay, "ACK_FREQUENCY"); frame::AckFrequency { sequence: sequence_number, ack_eliciting_threshold: config.ack_eliciting_threshold, - request_max_ack_delay: max_ack_delay_micros, + request_max_ack_delay: max_ack_delay.as_micros().try_into().unwrap_or(VarInt::MAX), reordering_threshold: config.reordering_threshold, } .encode(buf);