Fix gap filling ACK logic in QuicReceivedPacketManager. Updates the logic for sending immediate ACKs when receiving out-of-order packets. Previously, any packet smaller than last_sent_largest_acked_ would trigger an immediate ACK, regardless of the threshold. This CL ensures compliance with draft-ietf-quic-ack-frequency-13 Section 6.2 by respecting the reordering_threshold: immediate ACKs are now only triggered if the received packet is outside the reordering window (i.e., less than or equal to last_sent_largest_acked_ - reordering_threshold_). This reduces the number of immediate ACKs sent on networks with packet reordering, improving efficiency. Handles underflow scenarios: if last_sent_largest_acked_ is smaller than the reordering_threshold_, no immediate ACK is sent, preventing incorrect immediate ACKs when the reordering threshold exceeds the largest acked packet. Protected by default disabled --quic_reloadable_flag_quic_fix_gap_filling_ack_logic. PiperOrigin-RevId: 853138944
diff --git a/quiche/common/quiche_feature_flags_list.h b/quiche/common/quiche_feature_flags_list.h index 7b26e5d..57bc31e 100755 --- a/quiche/common/quiche_feature_flags_list.h +++ b/quiche/common/quiche_feature_flags_list.h
@@ -41,6 +41,7 @@ QUICHE_FLAG(bool, quiche_reloadable_flag_quic_enforce_immediate_goaway, false, true, "If true, QUIC will support sending immediate GOAWAYS and will refuse streams above the limit.") QUICHE_FLAG(bool, quiche_reloadable_flag_quic_enobufs_blocked, true, true, "If true, ENOBUFS socket errors are reported as socket blocked instead of socket failure.") QUICHE_FLAG(bool, quiche_reloadable_flag_quic_fin_before_completed_http_headers, false, true, "If true, close the connection with error if FIN is received before finish receiving the whole HTTP headers.") +QUICHE_FLAG(bool, quiche_reloadable_flag_quic_fix_gap_filling_ack_logic, false, false, "If true, fix the gap filling ack logic in QuicAckFrame.") QUICHE_FLAG(bool, quiche_reloadable_flag_quic_fix_timeouts, true, true, "If true, postpone setting handshake timeout to infinite to handshake complete.") QUICHE_FLAG(bool, quiche_reloadable_flag_quic_include_datagrams_in_willing_to_write, false, false, "If true, checks for queued datagrams when determining if a connection is willing to write.") QUICHE_FLAG(bool, quiche_reloadable_flag_quic_limit_new_streams_per_loop_2, true, true, "If true, when the peer sends connection options \\\'SLP1\\\', \\\'SLP2\\\' and \\\'SLPF\\\', internet facing GFEs will only allow a limited number of new requests to be processed per event loop, and postpone the rest to the following event loops. Also guard QuicConnection to iterate through all decrypters at each encryption level to get cipher id for a request.")
diff --git a/quiche/quic/core/quic_received_packet_manager.cc b/quiche/quic/core/quic_received_packet_manager.cc index eb3449b..5b78319 100644 --- a/quiche/quic/core/quic_received_packet_manager.cc +++ b/quiche/quic/core/quic_received_packet_manager.cc
@@ -295,16 +295,27 @@ return; } - // Limiting this to reordering_threshold_ > 0 is not compliant with - // draft-ietf-quic-ack-frequency-11, but there is an issue to add this - // behavior. - if (reordering_threshold_ > 0 && was_last_packet_missing_ && - last_sent_largest_acked_.IsInitialized() && - last_received_packet_number < last_sent_largest_acked_) { - // Ack immediately if an ACK frame was sent with a larger largest acked than - // the newly received packet number. - ack_timeout_ = now; - return; + if (GetQuicReloadableFlag(quic_fix_gap_filling_ack_logic)) { + QUIC_RELOADABLE_FLAG_COUNT(quic_fix_gap_filling_ack_logic); + if (reordering_threshold_ > 0 && was_last_packet_missing_ && + last_sent_largest_acked_.IsInitialized() && + last_sent_largest_acked_ >= QuicPacketNumber(reordering_threshold_) && + (last_received_packet_number <= + last_sent_largest_acked_ - reordering_threshold_)) { + // Ack immediately if the received packet number is less than or equal to + // largest acked - reordering threshold. + ack_timeout_ = now; + return; + } + } else { + if (reordering_threshold_ > 0 && was_last_packet_missing_ && + last_sent_largest_acked_.IsInitialized() && + last_received_packet_number < last_sent_largest_acked_) { + // Ack immediately if an ACK frame was sent with a larger + // largest acked than the newly received packet number. + ack_timeout_ = now; + return; + } } if (changed_to_ce_marked_) {
diff --git a/quiche/quic/core/quic_received_packet_manager_test.cc b/quiche/quic/core/quic_received_packet_manager_test.cc index 2c33d2c..ffca6fa 100644 --- a/quiche/quic/core/quic_received_packet_manager_test.cc +++ b/quiche/quic/core/quic_received_packet_manager_test.cc
@@ -823,6 +823,197 @@ CheckAckTimeout(clock_.ApproximateNow()); } +TEST_F(QuicReceivedPacketManagerTest, LegacyLogicIgnoresReorderingThreshold) { + SetQuicReloadableFlag(quic_fix_gap_filling_ack_logic, false); + + // Setup a reordering threshold of 10. + QuicAckFrequencyFrame frame; + frame.sequence_number = 1; + frame.requested_max_ack_delay = kDelayedAckTime; + frame.ack_eliciting_threshold = 1000; + frame.reordering_threshold = 10; + received_manager_.OnAckFrequencyFrame(frame); + + // Establish a baseline: We have ACKed up to 100. + RecordPacketReceipt(100, clock_.ApproximateNow()); + received_manager_.GetUpdatedAckFrame(QuicTime::Zero()); + received_manager_.ResetAckStates(); // last_sent_largest_acked_ = 100 + + // Receive Packet 95. + // Legacy Logic: Is 95 < 100? YES -> Immediate ACK. + // (New Logic would say: Gap is 5, Threshold is 10 -> Delay ACK). + RecordPacketReceipt(95, clock_.ApproximateNow()); + MaybeUpdateAckTimeout(kInstigateAck, 95); + + // Verify Immediate ACK (The inefficient legacy behavior). + CheckAckTimeout(clock_.ApproximateNow()); +} + +TEST_F(QuicReceivedPacketManagerTest, ReorderingThresholdTriggersImmediateAck) { + SetQuicReloadableFlag(quic_fix_gap_filling_ack_logic, true); + + QuicAckFrequencyFrame frame; + frame.sequence_number = 1; + frame.requested_max_ack_delay = kDelayedAckTime; + frame.ack_eliciting_threshold = 1000; + frame.reordering_threshold = 10; + received_manager_.OnAckFrequencyFrame(frame); + + RecordPacketReceipt(100, clock_.ApproximateNow()); + received_manager_.GetUpdatedAckFrame(QuicTime::Zero()); + + // This sets last_sent_largest_acked_ = 100 + received_manager_.ResetAckStates(); + + // Receive Packet 90 (Matches boundary: 100 - 10 = 90) + RecordPacketReceipt(90, clock_.ApproximateNow()); + + MaybeUpdateAckTimeout(kInstigateAck, 90); + CheckAckTimeout(clock_.ApproximateNow()); +} + +TEST_F(QuicReceivedPacketManagerTest, + ReorderingThresholdDelaysAckForRecentPackets) { + SetQuicReloadableFlag(quic_fix_gap_filling_ack_logic, true); + QuicAckFrequencyFrame frame; + frame.sequence_number = 1; + frame.requested_max_ack_delay = kDelayedAckTime; + frame.ack_eliciting_threshold = 1000; + frame.reordering_threshold = 10; + received_manager_.OnAckFrequencyFrame(frame); + + RecordPacketReceipt(100, clock_.ApproximateNow()); + received_manager_.GetUpdatedAckFrame(QuicTime::Zero()); + + // Simulate that the ACK for 100 was actually SENT. + received_manager_.ResetAckStates(); + + // Receive a packet RECENT (inside boundary) + // Packet 95 > (100 - 10 = 90). Should NOT trigger immediate ACK. + RecordPacketReceipt(95, clock_.ApproximateNow()); + MaybeUpdateAckTimeout(kInstigateAck, 95); + + CheckAckTimeout(clock_.ApproximateNow() + kDelayedAckTime); +} + +TEST_F(QuicReceivedPacketManagerTest, ReorderingThresholdUnderflowGuard) { + SetQuicReloadableFlag(quic_fix_gap_filling_ack_logic, true); + + QuicAckFrequencyFrame frame; + frame.sequence_number = 1; + frame.requested_max_ack_delay = kDelayedAckTime; + frame.ack_eliciting_threshold = 1000; + frame.reordering_threshold = 10; + received_manager_.OnAckFrequencyFrame(frame); + + // Largest Acked (5) is SMALLER than threshold (10) + RecordPacketReceipt(5, clock_.ApproximateNow()); + received_manager_.GetUpdatedAckFrame(QuicTime::Zero()); + + received_manager_.ResetAckStates(); + + // (5 > 10) -> FALSE. Boundary stays 0. + // (3 <= 0) -> FALSE. + // Result: Delayed ACK. + RecordPacketReceipt(3, clock_.ApproximateNow()); + MaybeUpdateAckTimeout(kInstigateAck, 3); + + CheckAckTimeout(clock_.ApproximateNow() + kDelayedAckTime); +} + +TEST_F(QuicReceivedPacketManagerTest, ReorderingThresholdOneTriggersAck) { + SetQuicReloadableFlag(quic_fix_gap_filling_ack_logic, true); + + QuicAckFrequencyFrame frame; + frame.sequence_number = 1; + frame.requested_max_ack_delay = kDelayedAckTime; + frame.ack_eliciting_threshold = 1000; + frame.reordering_threshold = 1; + received_manager_.OnAckFrequencyFrame(frame); + + RecordPacketReceipt(100, clock_.ApproximateNow()); + received_manager_.GetUpdatedAckFrame(QuicTime::Zero()); + received_manager_.ResetAckStates(); + + // Receive Packet 99 (100 - 1 = 99). Should trigger immediate ACK. + // Condition: 99 <= (100 - 1) -> True. + RecordPacketReceipt(99, clock_.ApproximateNow()); + MaybeUpdateAckTimeout(kInstigateAck, 99); + CheckAckTimeout(clock_.ApproximateNow()); +} + +TEST_F(QuicReceivedPacketManagerTest, + ReorderingThresholdBoundaryPlusOneDelaysAck) { + SetQuicReloadableFlag(quic_fix_gap_filling_ack_logic, true); + + QuicAckFrequencyFrame frame; + frame.sequence_number = 1; + frame.requested_max_ack_delay = kDelayedAckTime; + frame.ack_eliciting_threshold = 1000; + frame.reordering_threshold = 10; + received_manager_.OnAckFrequencyFrame(frame); + + RecordPacketReceipt(100, clock_.ApproximateNow()); + received_manager_.GetUpdatedAckFrame(QuicTime::Zero()); + received_manager_.ResetAckStates(); + + // Receive Packet 91 (100 - 10 = 90). 91 > 90. + // Should NOT trigger immediate ACK. + RecordPacketReceipt(91, clock_.ApproximateNow()); + MaybeUpdateAckTimeout(kInstigateAck, 91); + CheckAckTimeout(clock_.ApproximateNow() + kDelayedAckTime); +} + +TEST_F(QuicReceivedPacketManagerTest, + ReorderingThresholdUnderflowGuardsPacketZero) { + SetQuicReloadableFlag(quic_fix_gap_filling_ack_logic, true); + + QuicAckFrequencyFrame frame; + frame.sequence_number = 1; + frame.requested_max_ack_delay = kDelayedAckTime; + frame.ack_eliciting_threshold = 1000; + frame.reordering_threshold = 10; + received_manager_.OnAckFrequencyFrame(frame); + + // Largest Acked = 5. Threshold = 10. + // Effective Boundary = 5 - 10 = -5. + RecordPacketReceipt(5, clock_.ApproximateNow()); + received_manager_.GetUpdatedAckFrame(QuicTime::Zero()); + received_manager_.ResetAckStates(); + + // Receive Packet 0. + // Condition: 0 <= -5 -> False. + // Should NOT trigger immediate ACK. + RecordPacketReceipt(0, clock_.ApproximateNow()); + MaybeUpdateAckTimeout(kInstigateAck, 0); + CheckAckTimeout(clock_.ApproximateNow() + kDelayedAckTime); +} + +TEST_F(QuicReceivedPacketManagerTest, + ReorderingThresholdExactZeroBoundaryTriggersAck) { + SetQuicReloadableFlag(quic_fix_gap_filling_ack_logic, true); + + QuicAckFrequencyFrame frame; + frame.sequence_number = 1; + frame.requested_max_ack_delay = kDelayedAckTime; + frame.ack_eliciting_threshold = 1000; + frame.reordering_threshold = 10; + received_manager_.OnAckFrequencyFrame(frame); + + // Largest Acked = 10. Threshold = 10. + // Effective Boundary = 10 - 10 = 0. + RecordPacketReceipt(10, clock_.ApproximateNow()); + received_manager_.GetUpdatedAckFrame(QuicTime::Zero()); + received_manager_.ResetAckStates(); + + // Receive Packet 0. + // Condition: 0 <= 0 -> True. + // Should trigger immediate ACK. + RecordPacketReceipt(0, clock_.ApproximateNow()); + MaybeUpdateAckTimeout(kInstigateAck, 0); + CheckAckTimeout(clock_.ApproximateNow()); +} + } // namespace } // namespace test