gfe-relnote: Stop removing handshake packets from flight when the handshake timer causes them to be retransmitted. Protected by gfe2_reloadable_flag_quic_loss_removes_from_inflight. PiperOrigin-RevId: 244887395 Change-Id: I355da0f1a78e596e4d4bd2a2721c37d1cedc5806
diff --git a/quic/core/quic_sent_packet_manager.cc b/quic/core/quic_sent_packet_manager.cc index 5a9cbb0..ad5f297 100644 --- a/quic/core/quic_sent_packet_manager.cc +++ b/quic/core/quic_sent_packet_manager.cc
@@ -111,10 +111,15 @@ QuicTime::Delta::FromMilliseconds(kDefaultDelayedAckTimeMs)), rtt_updated_(false), acked_packets_iter_(last_ack_frame_.packets.rbegin()), - tolerate_reneging_(GetQuicReloadableFlag(quic_tolerate_reneging)) { + tolerate_reneging_(GetQuicReloadableFlag(quic_tolerate_reneging)), + loss_removes_from_inflight_( + GetQuicReloadableFlag(quic_loss_removes_from_inflight)) { if (tolerate_reneging_) { QUIC_RELOADABLE_FLAG_COUNT(quic_tolerate_reneging); } + if (loss_removes_from_inflight_) { + QUIC_RELOADABLE_FLAG_COUNT(quic_loss_removes_from_inflight); + } SetSendAlgorithm(congestion_control_type); } @@ -357,14 +362,25 @@ DCHECK(retransmission_type == ALL_UNACKED_RETRANSMISSION || retransmission_type == ALL_INITIAL_RETRANSMISSION); QuicPacketNumber packet_number = unacked_packets_.GetLeastUnacked(); - for (QuicUnackedPacketMap::const_iterator it = unacked_packets_.begin(); + for (QuicUnackedPacketMap::iterator it = unacked_packets_.begin(); it != unacked_packets_.end(); ++it, ++packet_number) { if ((retransmission_type == ALL_UNACKED_RETRANSMISSION || - it->encryption_level == ENCRYPTION_ZERO_RTT) && - unacked_packets_.HasRetransmittableFrames(*it)) { - MarkForRetransmission(packet_number, retransmission_type); + it->encryption_level == ENCRYPTION_ZERO_RTT)) { + if (loss_removes_from_inflight_ && it->in_flight) { + // Remove 0-RTT packets and packets of the wrong version from flight, + // because neither can be processed by the peer. + unacked_packets_.RemoveFromInFlight(&*it); + } + if (unacked_packets_.HasRetransmittableFrames(*it)) { + MarkForRetransmission(packet_number, retransmission_type); + } } } + if (retransmission_type == ALL_UNACKED_RETRANSMISSION && + unacked_packets_.bytes_in_flight() > 0) { + QUIC_BUG << "RetransmitUnackedPackets should remove all packets from flight" + << ", bytes_in_flight:" << unacked_packets_.bytes_in_flight(); + } } void QuicSentPacketManager::NeuterUnencryptedPackets() { @@ -384,15 +400,17 @@ } for (QuicUnackedPacketMap::const_iterator it = unacked_packets_.begin(); it != unacked_packets_.end(); ++it, ++packet_number) { - if (it->encryption_level == ENCRYPTION_INITIAL && - unacked_packets_.HasRetransmittableFrames(*it)) { - // Once you're forward secure, no unencrypted packets will be sent, crypto - // or otherwise. Unencrypted packets are neutered and abandoned, to ensure - // they are not retransmitted or considered lost from a congestion control - // perspective. - pending_retransmissions_.erase(packet_number); - unacked_packets_.RemoveFromInFlight(packet_number); - unacked_packets_.RemoveRetransmittability(packet_number); + if (it->encryption_level == ENCRYPTION_INITIAL) { + if (loss_removes_from_inflight_ || + unacked_packets_.HasRetransmittableFrames(*it)) { + // Once you're forward secure, no unencrypted packets will be sent, + // crypto or otherwise. Unencrypted packets are neutered and abandoned, + // to ensure they are not retransmitted or considered lost from a + // congestion control perspective. + pending_retransmissions_.erase(packet_number); + unacked_packets_.RemoveFromInFlight(packet_number); + unacked_packets_.RemoveRetransmittability(packet_number); + } } } } @@ -437,7 +455,8 @@ // Handshake packets should never be sent as probing retransmissions. DCHECK(!transmission_info->has_crypto_handshake || transmission_type != PROBING_RETRANSMISSION); - if (!RetransmissionLeavesBytesInFlight(transmission_type)) { + if (!loss_removes_from_inflight_ && + !RetransmissionLeavesBytesInFlight(transmission_type)) { unacked_packets_.RemoveFromInFlight(transmission_info); } @@ -854,6 +873,9 @@ time); } + if (loss_removes_from_inflight_) { + unacked_packets_.RemoveFromInFlight(packet.packet_number); + } MarkForRetransmission(packet.packet_number, LOSS_RETRANSMISSION); } }
diff --git a/quic/core/quic_sent_packet_manager.h b/quic/core/quic_sent_packet_manager.h index f7f3c12..f2524a1 100644 --- a/quic/core/quic_sent_packet_manager.h +++ b/quic/core/quic_sent_packet_manager.h
@@ -629,6 +629,9 @@ // Latched value of quic_tolerate_reneging. const bool tolerate_reneging_; + + // Latched value of quic_loss_removes_from_inflight. + const bool loss_removes_from_inflight_; }; } // namespace quic
diff --git a/quic/core/quic_sent_packet_manager_test.cc b/quic/core/quic_sent_packet_manager_test.cc index 6448419..ec8de8b 100644 --- a/quic/core/quic_sent_packet_manager_test.cc +++ b/quic/core/quic_sent_packet_manager_test.cc
@@ -1036,6 +1036,7 @@ SendDataPacket(kNumSentCryptoPackets + i); } EXPECT_TRUE(manager_.HasUnackedCryptoPackets()); + EXPECT_EQ(5 * kDefaultLength, manager_.GetBytesInFlight()); // The first retransmits 2 packets. if (manager_.session_decides_what_to_write()) { @@ -1051,6 +1052,10 @@ RetransmitNextPacket(7); EXPECT_FALSE(manager_.HasPendingRetransmissions()); } + // Expect all 4 handshake packets to be in flight and 3 data packets. + if (GetQuicReloadableFlag(quic_loss_removes_from_inflight)) { + EXPECT_EQ(7 * kDefaultLength, manager_.GetBytesInFlight()); + } EXPECT_TRUE(manager_.HasUnackedCryptoPackets()); // The second retransmits 2 packets. @@ -1067,12 +1072,26 @@ RetransmitNextPacket(9); EXPECT_FALSE(manager_.HasPendingRetransmissions()); } + if (GetQuicReloadableFlag(quic_loss_removes_from_inflight)) { + EXPECT_EQ(9 * kDefaultLength, manager_.GetBytesInFlight()); + } EXPECT_TRUE(manager_.HasUnackedCryptoPackets()); // Now ack the two crypto packets and the speculatively encrypted request, // and ensure the first four crypto packets get abandoned, but not lost. - uint64_t acked[] = {3, 4, 5, 8, 9}; - ExpectAcksAndLosses(true, acked, QUIC_ARRAYSIZE(acked), nullptr, 0); + if (GetQuicReloadableFlag(quic_loss_removes_from_inflight)) { + // Crypto packets remain in flight, so any that aren't acked will be lost. + uint64_t acked[] = {3, 4, 5, 8, 9}; + uint64_t lost[] = {1, 2, 6}; + ExpectAcksAndLosses(true, acked, QUIC_ARRAYSIZE(acked), lost, + QUIC_ARRAYSIZE(lost)); + if (manager_.session_decides_what_to_write()) { + EXPECT_CALL(notifier_, OnFrameLost(_)).Times(3); + } + } else { + uint64_t acked[] = {3, 4, 5, 8, 9}; + ExpectAcksAndLosses(true, acked, QUIC_ARRAYSIZE(acked), nullptr, 0); + } if (manager_.session_decides_what_to_write()) { EXPECT_CALL(notifier_, HasUnackedCryptoData()) .WillRepeatedly(Return(false)); @@ -1203,8 +1222,13 @@ manager_.OnAckFrameEnd(clock_.Now(), ENCRYPTION_INITIAL)); EXPECT_FALSE(manager_.HasUnackedCryptoPackets()); - uint64_t unacked[] = {3}; - VerifyUnackedPackets(unacked, QUIC_ARRAYSIZE(unacked)); + if (GetQuicReloadableFlag(quic_loss_removes_from_inflight)) { + uint64_t unacked[] = {1, 3}; + VerifyUnackedPackets(unacked, QUIC_ARRAYSIZE(unacked)); + } else { + uint64_t unacked[] = {3}; + VerifyUnackedPackets(unacked, QUIC_ARRAYSIZE(unacked)); + } } TEST_P(QuicSentPacketManagerTest, CryptoHandshakeTimeoutUnsentDataPacket) {