gfe-relnote: In QUIC, when detecting loss of a packet number space, do not use no packet missing optimization if largest_newly_acked != the largest packet in packets_acked. Protected by gfe2_reloadable_flag_quic_fix_packets_acked. PiperOrigin-RevId: 253292739 Change-Id: I64e94f8eb6b256dbf47d4677732eb6e21bb78268
diff --git a/quic/core/congestion_control/general_loss_algorithm.cc b/quic/core/congestion_control/general_loss_algorithm.cc index 8cd5e71..cf4bc35 100644 --- a/quic/core/congestion_control/general_loss_algorithm.cc +++ b/quic/core/congestion_control/general_loss_algorithm.cc
@@ -66,8 +66,16 @@ loss_detection_timeout_ = QuicTime::Zero(); if (!packets_acked.empty() && packets_acked.front().packet_number == least_in_flight_) { - if (least_in_flight_ + packets_acked.size() - 1 == largest_newly_acked) { - // Optimization for the case when no packet is missing. + if (GetQuicReloadableFlag(quic_fix_packets_acked)) { + QUIC_RELOADABLE_FLAG_COUNT(quic_fix_packets_acked); + } + if ((!GetQuicReloadableFlag(quic_fix_packets_acked) || + packets_acked.back().packet_number == largest_newly_acked) && + least_in_flight_ + packets_acked.size() - 1 == largest_newly_acked) { + // Optimization for the case when no packet is missing. Please note, + // packets_acked can include packets of different packet number space, so + // do not use this optimization if largest_newly_acked is not the largest + // packet in packets_acked. least_in_flight_ = largest_newly_acked + 1; largest_previously_acked_ = largest_newly_acked; return;
diff --git a/quic/core/congestion_control/uber_loss_algorithm_test.cc b/quic/core/congestion_control/uber_loss_algorithm_test.cc index 1fe3465..307d791 100644 --- a/quic/core/congestion_control/uber_loss_algorithm_test.cc +++ b/quic/core/congestion_control/uber_loss_algorithm_test.cc
@@ -159,6 +159,40 @@ VerifyLosses(5, packets_acked_, std::vector<uint64_t>{2, 3}); } +// Regression test for b/133771183. +TEST_F(UberLossAlgorithmTest, PacketInLimbo) { + // This test mimics a scenario: server sends 1-SHLO, 2-1RTT, 3-1RTT, + // 4-retransmit SHLO. Client receives and ACKs packets 1, 3 and 4. + QuicUnackedPacketMapPeer::SetPerspective(unacked_packets_.get(), + Perspective::IS_SERVER); + + SendPacket(1, ENCRYPTION_ZERO_RTT); + SendPacket(2, ENCRYPTION_FORWARD_SECURE); + SendPacket(3, ENCRYPTION_FORWARD_SECURE); + SendPacket(4, ENCRYPTION_ZERO_RTT); + + SendPacket(5, ENCRYPTION_FORWARD_SECURE); + AckPackets({1, 3, 4}); + unacked_packets_->MaybeUpdateLargestAckedOfPacketNumberSpace( + APPLICATION_DATA, QuicPacketNumber(3)); + unacked_packets_->MaybeUpdateLargestAckedOfPacketNumberSpace( + HANDSHAKE_DATA, QuicPacketNumber(4)); + // No packet loss detected. + VerifyLosses(4, packets_acked_, std::vector<uint64_t>{}); + + SendPacket(6, ENCRYPTION_FORWARD_SECURE); + AckPackets({5, 6}); + unacked_packets_->MaybeUpdateLargestAckedOfPacketNumberSpace( + APPLICATION_DATA, QuicPacketNumber(6)); + if (GetQuicReloadableFlag(quic_fix_packets_acked)) { + // Verify packet 2 is detected lost. + VerifyLosses(6, packets_acked_, std::vector<uint64_t>{2}); + } else { + // No losses, packet 2 is in limbo. + VerifyLosses(6, packets_acked_, std::vector<uint64_t>{}); + } +} + } // namespace } // namespace test } // namespace quic
diff --git a/quic/core/quic_sent_packet_manager_test.cc b/quic/core/quic_sent_packet_manager_test.cc index 1e8302f..58d8b14 100644 --- a/quic/core/quic_sent_packet_manager_test.cc +++ b/quic/core/quic_sent_packet_manager_test.cc
@@ -68,16 +68,23 @@ HANDSHAKE_RETRANSMISSION, HAS_RETRANSMITTABLE_DATA); } - void RetransmitDataPacket(uint64_t packet_number, TransmissionType type) { + void RetransmitDataPacket(uint64_t packet_number, + TransmissionType type, + EncryptionLevel level) { EXPECT_CALL( *send_algorithm_, OnPacketSent(_, BytesInFlight(), QuicPacketNumber(packet_number), kDefaultLength, HAS_RETRANSMITTABLE_DATA)); SerializedPacket packet(CreatePacket(packet_number, true)); + packet.encryption_level = level; manager_.OnPacketSent(&packet, QuicPacketNumber(), clock_.Now(), type, HAS_RETRANSMITTABLE_DATA); } + void RetransmitDataPacket(uint64_t packet_number, TransmissionType type) { + RetransmitDataPacket(packet_number, type, ENCRYPTION_INITIAL); + } + protected: QuicSentPacketManagerTest() : manager_(Perspective::IS_SERVER, @@ -319,12 +326,19 @@ } void SendAckPacket(uint64_t packet_number, uint64_t largest_acked) { + SendAckPacket(packet_number, largest_acked, ENCRYPTION_INITIAL); + } + + void SendAckPacket(uint64_t packet_number, + uint64_t largest_acked, + EncryptionLevel level) { EXPECT_CALL( *send_algorithm_, OnPacketSent(_, BytesInFlight(), QuicPacketNumber(packet_number), kDefaultLength, NO_RETRANSMITTABLE_DATA)); SerializedPacket packet(CreatePacket(packet_number, false)); packet.largest_acked = QuicPacketNumber(largest_acked); + packet.encryption_level = level; manager_.OnPacketSent(&packet, QuicPacketNumber(), clock_.Now(), NOT_RETRANSMISSION, NO_RETRANSMITTABLE_DATA); } @@ -2830,6 +2844,65 @@ manager_.OnAckFrameEnd(clock_.Now(), ENCRYPTION_HANDSHAKE)); } +// Regression test for b/133771183. +TEST_P(QuicSentPacketManagerTest, PacketInLimbo) { + if (!manager_.session_decides_what_to_write()) { + return; + } + QuicSentPacketManagerPeer::SetMaxTailLossProbes(&manager_, 2); + // Send SHLO. + SendCryptoPacket(1); + // Send data packet. + SendDataPacket(2, ENCRYPTION_FORWARD_SECURE); + // Send Ack Packet. + SendAckPacket(3, 1, ENCRYPTION_FORWARD_SECURE); + // Retransmit SHLO. + EXPECT_CALL(notifier_, RetransmitFrames(_, _)) + .WillOnce(InvokeWithoutArgs([this]() { RetransmitCryptoPacket(4); })); + manager_.OnRetransmissionTimeout(); + + // Successfully decrypt a forward secure packet. + manager_.SetHandshakeConfirmed(); + EXPECT_CALL(notifier_, HasUnackedCryptoData()).WillRepeatedly(Return(false)); + // Send Ack packet. + SendAckPacket(5, 2, ENCRYPTION_FORWARD_SECURE); + + // Retransmission alarm fires. + manager_.OnRetransmissionTimeout(); + EXPECT_CALL(notifier_, RetransmitFrames(_, _)) + .WillOnce(WithArgs<1>(Invoke([this](TransmissionType type) { + RetransmitDataPacket(6, type, ENCRYPTION_FORWARD_SECURE); + }))); + manager_.MaybeRetransmitTailLossProbe(); + + // Received Ack of packets 1, 3 and 4. + uint64_t acked[] = {1, 3, 4}; + ExpectAcksAndLosses(true, acked, QUIC_ARRAYSIZE(acked), nullptr, 0); + manager_.OnAckFrameStart(QuicPacketNumber(4), QuicTime::Delta::Infinite(), + clock_.Now()); + manager_.OnAckRange(QuicPacketNumber(3), QuicPacketNumber(5)); + manager_.OnAckRange(QuicPacketNumber(1), QuicPacketNumber(2)); + EXPECT_EQ(PACKETS_NEWLY_ACKED, + manager_.OnAckFrameEnd(clock_.Now(), ENCRYPTION_INITIAL)); + + uint64_t acked2[] = {5, 6}; + uint64_t loss[] = {2}; + if (GetQuicReloadableFlag(quic_fix_packets_acked)) { + // Verify packet 2 is detected lost. + EXPECT_CALL(notifier_, OnFrameLost(_)).Times(1); + ExpectAcksAndLosses(true, acked2, QUIC_ARRAYSIZE(acked2), loss, + QUIC_ARRAYSIZE(loss)); + } else { + // Packet 2 in limbo, bummer. + ExpectAcksAndLosses(true, acked2, QUIC_ARRAYSIZE(acked2), nullptr, 0); + } + manager_.OnAckFrameStart(QuicPacketNumber(6), QuicTime::Delta::Infinite(), + clock_.Now()); + manager_.OnAckRange(QuicPacketNumber(3), QuicPacketNumber(7)); + EXPECT_EQ(PACKETS_NEWLY_ACKED, + manager_.OnAckFrameEnd(clock_.Now(), ENCRYPTION_INITIAL)); +} + } // namespace } // namespace test } // namespace quic