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
