diff --git a/quinn-proto/src/connection/mod.rs b/quinn-proto/src/connection/mod.rs index 0d24d12b3..64dca6934 100644 --- a/quinn-proto/src/connection/mod.rs +++ b/quinn-proto/src/connection/mod.rs @@ -725,13 +725,9 @@ where ); pad_datagram |= sent.requires_padding; - // If we sent any acks, don't immediately resend them. Setting this even if ack_only is - // false needlessly prevents us from ACKing the next packet if it's ACK-only, but saves - // the need for subtler logic to avoid double-transmitting acks all the time. - // This reset needs to happen before we check whether more data - // is available in this space - because otherwise it would return - // `true` purely due to the ACKs. - self.spaces[space_id].permit_ack_only &= sent.acks.is_empty(); + if !sent.acks.is_empty() { + self.spaces[space_id].pending_acks.acks_sent(); + } // Keep information about the packet around until it gets finalized sent_frames = Some(sent); @@ -1456,9 +1452,6 @@ where } let space = &mut self.spaces[space_id]; space.pending_acks.insert_one(packet); - if space.pending_acks.len() > MAX_ACK_BLOCKS { - space.pending_acks.pop_min(); - } if packet >= space.rx_packet { space.rx_packet = packet; // Update outgoing spin bit, inverting iff we're the client @@ -2166,7 +2159,9 @@ where match frame { Frame::Ack(_) | Frame::Padding | Frame::Close(Close::Connection(_)) => {} _ => { - self.spaces[packet.header.space()].permit_ack_only = true; + self.spaces[packet.header.space()] + .pending_acks + .ack_eliciting_frame_received(); } } // Process frames @@ -2252,7 +2247,9 @@ where match frame { Frame::Ack(_) | Frame::Padding | Frame::Close(_) => {} _ => { - self.spaces[SpaceId::Data].permit_ack_only = true; + self.spaces[SpaceId::Data] + .pending_acks + .ack_eliciting_frame_received(); } } // Check whether this could be a probing packet @@ -2603,7 +2600,7 @@ where // ACK // 0-RTT packets must never carry acks (which would have to be of handshake packets) - if !space.pending_acks.is_empty() { + 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 { @@ -2611,8 +2608,8 @@ where } else { None }; - frame::Ack::encode(0, &space.pending_acks, ecn, buf); - sent.acks = space.pending_acks.clone(); + sent.acks = space.pending_acks.ranges().clone(); + frame::Ack::encode(0, &sent.acks, ecn, buf); self.stats.frame_tx.acks += 1; } @@ -3121,9 +3118,6 @@ mod state { } } -/// Ensures we can always fit all our ACKs in a single minimum-MTU packet with room to spare -const MAX_ACK_BLOCKS: usize = 64; - struct PrevCrypto where K: crypto::PacketKey, diff --git a/quinn-proto/src/connection/spaces.rs b/quinn-proto/src/connection/spaces.rs index 377c31549..b67706d9b 100644 --- a/quinn-proto/src/connection/spaces.rs +++ b/quinn-proto/src/connection/spaces.rs @@ -26,9 +26,7 @@ where /// Data to send pub(crate) pending: Retransmits, /// Packet numbers to acknowledge - pub(crate) pending_acks: ArrayRangeSet, - /// Set iff we have received a non-ack frame since the last ack-only packet we sent - pub(crate) permit_ack_only: bool, + pub(crate) pending_acks: PendingAcks, /// The packet number of the next packet that will be sent, if any. pub(crate) next_packet_number: u64, @@ -79,8 +77,7 @@ where rx_packet: 0, pending: Retransmits::default(), - pending_acks: ArrayRangeSet::new(), - permit_ack_only: false, + pending_acks: PendingAcks::default(), next_packet_number: 0, largest_acked_packet: None, @@ -149,7 +146,7 @@ where } pub(crate) fn can_send(&self) -> SendableFrames { - let acks = self.permit_ack_only && !self.pending_acks.is_empty(); + let acks = self.pending_acks.can_send(); let other = !self.pending.is_empty() || self.ping_pending; SendableFrames { acks, other } @@ -436,6 +433,63 @@ impl SendableFrames { } } +#[derive(Debug, Default)] +pub(crate) struct PendingAcks { + permit_ack_only: bool, + ranges: ArrayRangeSet, +} + +impl PendingAcks { + /// Whether any ACK frames can be sent + pub fn can_send(&self) -> bool { + self.permit_ack_only && !self.ranges.is_empty() + } + + /// Should be called whenever an ACK eliciting frame was received + /// + /// This requires sending new outgoing ACKs + pub fn ack_eliciting_frame_received(&mut self) { + self.permit_ack_only = true; + } + + /// Should be called whenever ACKs have been sent + /// + /// This will suppress sending further ACKs until additional ACK eliciting frames arrive + pub fn acks_sent(&mut self) { + // If we sent any acks, don't immediately resend them. Setting this even if ack_only is + // false needlessly prevents us from ACKing the next packet if it's ACK-only, but saves + // the need for subtler logic to avoid double-transmitting acks all the time. + // This reset needs to happen before we check whether more data + // is available in this space - because otherwise it would return + // `true` purely due to the ACKs + self.permit_ack_only = false; + } + + /// Insert one packet that needs to be acknowledged + pub fn insert_one(&mut self, packet: u64) { + self.ranges.insert_one(packet); + if self.ranges.len() > MAX_ACK_BLOCKS { + self.ranges.pop_min(); + } + } + + /// Removes the given ACKs from the set of pending ACKs + pub fn subtract(&mut self, acks: &ArrayRangeSet) { + self.ranges.subtract(acks); + if self.ranges.is_empty() { + self.permit_ack_only = false; + } + } + + /// Returns the set of currently pending ACK ranges + pub fn ranges(&self) -> &ArrayRangeSet { + &self.ranges + } +} + +/// Ensures we can always fit all our ACKs in a single minimum-MTU packet with room to spare +const MAX_ACK_BLOCKS: usize = 64; + #[cfg(test)] mod test { use super::*;