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) {
