diff --git a/quinn-proto/src/connection/mod.rs b/quinn-proto/src/connection/mod.rs index 64dca6934..171b3a389 100644 --- a/quinn-proto/src/connection/mod.rs +++ b/quinn-proto/src/connection/mod.rs @@ -1451,7 +1451,7 @@ where } } let space = &mut self.spaces[space_id]; - space.pending_acks.insert_one(packet); + space.pending_acks.insert_one(packet, now); if packet >= space.rx_packet { space.rx_packet = packet; // Update outgoing spin bit, inverting iff we're the client @@ -2599,18 +2599,8 @@ where } // ACK - // 0-RTT packets must never carry acks (which would have to be of handshake packets) if !space.pending_acks.ranges().is_empty() { - debug_assert!(space.crypto.is_some(), "tried to send ACK in 0-RTT"); - trace!("ACK"); - let ecn = if self.receiving_ecn { - Some(&space.ecn_counters) - } else { - None - }; - sent.acks = space.pending_acks.ranges().clone(); - frame::Ack::encode(0, &sent.acks, ecn, buf); - self.stats.frame_tx.acks += 1; + Self::populate_acks(self.receiving_ecn, &mut sent, space, buf, &mut self.stats); } // PATH_CHALLENGE @@ -2747,6 +2737,40 @@ where sent } + /// Write pending ACKs into a buffer + /// + /// This method assumes ACKs are pending, and should only be called if + /// `!PendingAcks::ranges().is_empty()` returns `true`. + fn populate_acks( + receiving_ecn: bool, + sent: &mut SentFrames, + space: &mut PacketSpace, + buf: &mut Vec, + stats: &mut ConnectionStats, + ) { + debug_assert!(!space.pending_acks.ranges().is_empty()); + + // 0-RTT packets must never carry acks (which would have to be of handshake packets) + debug_assert!(space.crypto.is_some(), "tried to send ACK in 0-RTT"); + let ecn = if receiving_ecn { + Some(&space.ecn_counters) + } else { + None + }; + sent.acks = space.pending_acks.ranges().clone(); + + let delay_micros = space.pending_acks.ack_delay().as_micros() as u64; + + // TODO: This should come frome `TransportConfig` if that gets configurable + let ack_delay_exp = TransportParameters::default().ack_delay_exponent; + let delay = delay_micros >> ack_delay_exp.into_inner(); + + trace!("ACK {:?}, Delay = {}us", sent.acks, delay); + + frame::Ack::encode(delay as _, &sent.acks, ecn, buf); + stats.frame_tx.acks += 1; + } + fn close_common(&mut self) { trace!("connection closed"); for &timer in &Timer::VALUES { diff --git a/quinn-proto/src/connection/paths.rs b/quinn-proto/src/connection/paths.rs index 233f6eef4..db7179c3e 100644 --- a/quinn-proto/src/connection/paths.rs +++ b/quinn-proto/src/connection/paths.rs @@ -113,7 +113,7 @@ impl RttEstimator { self.min = cmp::min(self.min, self.latest); // Based on RFC6298. if let Some(smoothed) = self.smoothed { - let adjusted_rtt = if self.min + ack_delay < self.latest { + let adjusted_rtt = if self.min + ack_delay <= self.latest { self.latest - ack_delay } else { self.latest diff --git a/quinn-proto/src/connection/spaces.rs b/quinn-proto/src/connection/spaces.rs index b67706d9b..de2cca07e 100644 --- a/quinn-proto/src/connection/spaces.rs +++ b/quinn-proto/src/connection/spaces.rs @@ -3,7 +3,7 @@ use std::{ collections::{BTreeMap, VecDeque}, mem, ops::{Index, IndexMut}, - time::Instant, + time::{Duration, Instant}, }; use fxhash::FxHashSet; @@ -437,6 +437,12 @@ impl SendableFrames { pub(crate) struct PendingAcks { permit_ack_only: bool, ranges: ArrayRangeSet, + /// This value will be used for calculating ACK delay once it is implemented + /// + /// ACK delay will be the delay between when a packet arrived (`latest_incoming`) + /// and between it will be allowed to be acknowledged (`can_send() == true`). + latest_incoming: Option, + ack_delay: Duration, } impl PendingAcks { @@ -445,6 +451,11 @@ impl PendingAcks { self.permit_ack_only && !self.ranges.is_empty() } + /// Returns the duration the acknowledgement of the latest incoming packet has been delayed + pub fn ack_delay(&self) -> Duration { + self.ack_delay + } + /// Should be called whenever an ACK eliciting frame was received /// /// This requires sending new outgoing ACKs @@ -466,8 +477,10 @@ impl PendingAcks { } /// Insert one packet that needs to be acknowledged - pub fn insert_one(&mut self, packet: u64) { + pub fn insert_one(&mut self, packet: u64, now: Instant) { self.ranges.insert_one(packet); + self.latest_incoming = Some(now); + if self.ranges.len() > MAX_ACK_BLOCKS { self.ranges.pop_min(); } diff --git a/quinn-proto/src/tests/mod.rs b/quinn-proto/src/tests/mod.rs index 52e63b145..b3a440764 100644 --- a/quinn-proto/src/tests/mod.rs +++ b/quinn-proto/src/tests/mod.rs @@ -869,7 +869,7 @@ fn instant_close_2() { #[test] fn idle_timeout() { let _guard = subscribe(); - const IDLE_TIMEOUT: Duration = Duration::from_millis(10); + const IDLE_TIMEOUT: Duration = Duration::from_millis(100); let server = ServerConfig { transport: Arc::new(TransportConfig { max_idle_timeout: Some(IDLE_TIMEOUT), diff --git a/quinn-proto/src/tests/util.rs b/quinn-proto/src/tests/util.rs index 09f1038d5..c475dd8da 100644 --- a/quinn-proto/src/tests/util.rs +++ b/quinn-proto/src/tests/util.rs @@ -260,10 +260,10 @@ impl TestEndpoint { } while self.inbound.front().map_or(false, |x| x.0 <= now) { - let (_, ecn, packet) = self.inbound.pop_front().unwrap(); + let (recv_time, ecn, packet) = self.inbound.pop_front().unwrap(); if let Some((ch, event)) = self.endpoint - .handle(now, remote, None, ecn, packet.as_slice().into()) + .handle(recv_time, remote, None, ecn, packet.as_slice().into()) { match event { DatagramEvent::NewConnection(conn) => { diff --git a/quinn-proto/src/transport_parameters.rs b/quinn-proto/src/transport_parameters.rs index 4be1e3d27..30f59f830 100644 --- a/quinn-proto/src/transport_parameters.rs +++ b/quinn-proto/src/transport_parameters.rs @@ -139,7 +139,6 @@ impl TransportParameters { .try_into() .expect("setter guarantees this is in-bounds") }), - max_ack_delay: 0u32.into(), disable_active_migration: server_config.map_or(false, |c| !c.migration), active_connection_id_limit: if cid_gen.cid_len() == 0 { 2 // i.e. default, i.e. unsent