gfe-relnote: In QUIC, call NeuterHandshakePackets() at most once per connection. Protected by gfe2_reloadable_flag_quic_neuter_handshake_packets_once2 which replaces gfe2_reloadable_flag_quic_neuter_handshake_packets_once.
Also fix the bug where RTO and PTO try to retransmit not in flight packets.
PiperOrigin-RevId: 279118870
Change-Id: I1235372277109c18fded6c8d162a2147424d3dbd
diff --git a/quic/core/frames/quic_stream_frame.cc b/quic/core/frames/quic_stream_frame.cc
index c7626d3..d0e65d2 100644
--- a/quic/core/frames/quic_stream_frame.cc
+++ b/quic/core/frames/quic_stream_frame.cc
@@ -49,4 +49,8 @@
offset == rhs.offset;
}
+bool QuicStreamFrame::operator!=(const QuicStreamFrame& rhs) const {
+ return !(*this == rhs);
+}
+
} // namespace quic
diff --git a/quic/core/frames/quic_stream_frame.h b/quic/core/frames/quic_stream_frame.h
index 5e7439d..5c5323b 100644
--- a/quic/core/frames/quic_stream_frame.h
+++ b/quic/core/frames/quic_stream_frame.h
@@ -33,6 +33,8 @@
bool operator==(const QuicStreamFrame& rhs) const;
+ bool operator!=(const QuicStreamFrame& rhs) const;
+
bool fin;
QuicPacketLength data_length;
QuicStreamId stream_id;
diff --git a/quic/core/quic_sent_packet_manager.cc b/quic/core/quic_sent_packet_manager.cc
index c8c870b..cdc5a1c 100644
--- a/quic/core/quic_sent_packet_manager.cc
+++ b/quic/core/quic_sent_packet_manager.cc
@@ -107,7 +107,7 @@
pto_exponential_backoff_start_point_(0),
pto_rttvar_multiplier_(4),
neuter_handshake_packets_once_(
- GetQuicReloadableFlag(quic_neuter_handshake_packets_once)) {
+ GetQuicReloadableFlag(quic_neuter_handshake_packets_once2)) {
SetSendAlgorithm(congestion_control_type);
}
@@ -342,7 +342,7 @@
void QuicSentPacketManager::SetHandshakeConfirmed() {
if (!neuter_handshake_packets_once_ || !handshake_confirmed_) {
if (neuter_handshake_packets_once_) {
- QUIC_RELOADABLE_FLAG_COUNT(quic_neuter_handshake_packets_once);
+ QUIC_RELOADABLE_FLAG_COUNT_N(quic_neuter_handshake_packets_once2, 1, 3);
}
handshake_confirmed_ = true;
NeuterHandshakePackets();
@@ -454,7 +454,7 @@
void QuicSentPacketManager::NeuterUnencryptedPackets() {
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 (!it->retransmittable_frames.empty() &&
it->encryption_level == ENCRYPTION_INITIAL) {
@@ -462,18 +462,31 @@
// will be sent. The data has been abandoned in the cryto stream. Remove
// it from in flight.
unacked_packets_.RemoveFromInFlight(packet_number);
+ if (neuter_handshake_packets_once_) {
+ QUIC_RELOADABLE_FLAG_COUNT_N(quic_neuter_handshake_packets_once2, 2, 3);
+ it->state = NEUTERED;
+ DCHECK(!unacked_packets_.HasRetransmittableFrames(*it));
+ }
}
}
}
void QuicSentPacketManager::NeuterHandshakePackets() {
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 (!it->retransmittable_frames.empty() &&
unacked_packets_.GetPacketNumberSpace(it->encryption_level) ==
HANDSHAKE_DATA) {
unacked_packets_.RemoveFromInFlight(packet_number);
+ if (neuter_handshake_packets_once_) {
+ // Notify session that the data has been delivered (but do not notify
+ // send algorithm).
+ QUIC_RELOADABLE_FLAG_COUNT_N(quic_neuter_handshake_packets_once2, 3, 3);
+ it->state = NEUTERED;
+ unacked_packets_.NotifyFramesAcked(*it, QuicTime::Delta::Zero(),
+ QuicTime::Zero());
+ }
}
}
}
@@ -765,6 +778,7 @@
if (it->state == OUTSTANDING &&
unacked_packets_.HasRetransmittableFrames(*it) &&
pending_timer_transmission_count_ < max_rto_packets_) {
+ DCHECK(!neuter_handshake_packets_once_ || it->in_flight);
retransmissions.push_back(packet_number);
++pending_timer_transmission_count_;
}
@@ -797,6 +811,7 @@
it != unacked_packets_.end(); ++it, ++packet_number) {
if (it->state == OUTSTANDING &&
unacked_packets_.HasRetransmittableFrames(*it)) {
+ DCHECK(!neuter_handshake_packets_once_ || it->in_flight);
probing_packets.push_back(packet_number);
if (probing_packets.size() == pending_timer_transmission_count_) {
break;
diff --git a/quic/core/quic_sent_packet_manager.h b/quic/core/quic_sent_packet_manager.h
index 95b9ee2..67ded33 100644
--- a/quic/core/quic_sent_packet_manager.h
+++ b/quic/core/quic_sent_packet_manager.h
@@ -646,7 +646,7 @@
// The multiplier of rttvar when calculating PTO timeout.
int pto_rttvar_multiplier_;
- // Latched value of quic_neuter_handshake_packets_once.
+ // Latched value of quic_neuter_handshake_packets_once2.
const bool neuter_handshake_packets_once_;
};
diff --git a/quic/core/quic_sent_packet_manager_test.cc b/quic/core/quic_sent_packet_manager_test.cc
index 539ce97..1bd08d9 100644
--- a/quic/core/quic_sent_packet_manager_test.cc
+++ b/quic/core/quic_sent_packet_manager_test.cc
@@ -1125,9 +1125,9 @@
// Now neuter all unacked unencrypted packets, which occurs when the
// connection goes forward secure.
- manager_.NeuterUnencryptedPackets();
EXPECT_CALL(notifier_, HasUnackedCryptoData()).WillRepeatedly(Return(false));
EXPECT_CALL(notifier_, IsFrameOutstanding(_)).WillRepeatedly(Return(false));
+ manager_.NeuterUnencryptedPackets();
EXPECT_FALSE(manager_.HasUnackedCryptoPackets());
uint64_t unacked[] = {1, 2, 3};
VerifyUnackedPackets(unacked, QUIC_ARRAYSIZE(unacked));
@@ -3055,6 +3055,58 @@
manager_.GetRetransmissionTime());
}
+// Regression test for b/143962153
+TEST_F(QuicSentPacketManagerTest, RtoNotInFlightPacket) {
+ QuicSentPacketManagerPeer::SetMaxTailLossProbes(&manager_, 2);
+ // Send SHLO.
+ QuicStreamFrame crypto_frame(1, false, 0, QuicStringPiece());
+ SendCryptoPacket(1);
+ // Send data packet.
+ SendDataPacket(2, ENCRYPTION_FORWARD_SECURE);
+
+ // Successfully decrypt a forward secure packet.
+ if (GetQuicReloadableFlag(quic_neuter_handshake_packets_once2)) {
+ EXPECT_CALL(notifier_, OnFrameAcked(_, _, _)).Times(1);
+ } else {
+ EXPECT_CALL(notifier_, OnFrameAcked(_, _, _)).Times(0);
+ }
+ manager_.SetHandshakeConfirmed();
+
+ // 1st TLP.
+ manager_.OnRetransmissionTimeout();
+ EXPECT_CALL(notifier_, RetransmitFrames(_, _))
+ .WillOnce(WithArgs<1>(Invoke([this](TransmissionType type) {
+ RetransmitDataPacket(3, type, ENCRYPTION_FORWARD_SECURE);
+ })));
+ manager_.MaybeRetransmitTailLossProbe();
+
+ // 2nd TLP.
+ manager_.OnRetransmissionTimeout();
+ EXPECT_CALL(notifier_, RetransmitFrames(_, _))
+ .WillOnce(WithArgs<1>(Invoke([this](TransmissionType type) {
+ RetransmitDataPacket(4, type, ENCRYPTION_FORWARD_SECURE);
+ })));
+ manager_.MaybeRetransmitTailLossProbe();
+
+ // RTO retransmits SHLO although it is not in flight.
+ size_t num_rto_packets = 2;
+ if (GetQuicReloadableFlag(quic_neuter_handshake_packets_once2)) {
+ num_rto_packets = 1;
+ }
+ EXPECT_CALL(notifier_, RetransmitFrames(_, _))
+ .Times(num_rto_packets)
+ .WillOnce(
+ WithArgs<0>(Invoke([this, &crypto_frame](const QuicFrames& frames) {
+ EXPECT_EQ(1u, frames.size());
+ if (GetQuicReloadableFlag(quic_neuter_handshake_packets_once2)) {
+ EXPECT_NE(crypto_frame, frames[0].stream_frame);
+ } else {
+ EXPECT_EQ(crypto_frame, frames[0].stream_frame);
+ }
+ })));
+ manager_.OnRetransmissionTimeout();
+}
+
} // namespace
} // namespace test
} // namespace quic
diff --git a/quic/core/quic_types.h b/quic/core/quic_types.h
index 972e57c..06931dc 100644
--- a/quic/core/quic_types.h
+++ b/quic/core/quic_types.h
@@ -446,7 +446,7 @@
};
enum SentPacketState : uint8_t {
- // The packet has been sent and waiting to be acked.
+ // The packet is in flight and waiting to be acked.
OUTSTANDING,
FIRST_PACKET_STATE = OUTSTANDING,
// The packet was never sent.
@@ -455,6 +455,8 @@
ACKED,
// This packet is not expected to be acked.
UNACKABLE,
+ // This packet has been delivered or unneeded.
+ NEUTERED,
// States below are corresponding to retransmission types in TransmissionType.
diff --git a/quic/core/quic_utils.cc b/quic/core/quic_utils.cc
index a6f6577..eed0e37 100644
--- a/quic/core/quic_utils.cc
+++ b/quic/core/quic_utils.cc
@@ -161,6 +161,7 @@
RETURN_STRING_LITERAL(NEVER_SENT);
RETURN_STRING_LITERAL(ACKED);
RETURN_STRING_LITERAL(UNACKABLE);
+ RETURN_STRING_LITERAL(NEUTERED);
RETURN_STRING_LITERAL(HANDSHAKE_RETRANSMITTED);
RETURN_STRING_LITERAL(LOST);
RETURN_STRING_LITERAL(TLP_RETRANSMITTED);
diff --git a/quic/test_tools/simple_session_notifier.cc b/quic/test_tools/simple_session_notifier.cc
index 5ba12c6..9e67c89 100644
--- a/quic/test_tools/simple_session_notifier.cc
+++ b/quic/test_tools/simple_session_notifier.cc
@@ -126,8 +126,13 @@
}
void SimpleSessionNotifier::NeuterUnencryptedData() {
- // TODO(nharper): Handle CRYPTO frame case.
if (QuicVersionUsesCryptoFrames(connection_->transport_version())) {
+ for (const auto& interval : crypto_bytes_transferred_[ENCRYPTION_INITIAL]) {
+ QuicCryptoFrame crypto_frame(ENCRYPTION_INITIAL, interval.min(),
+ interval.max() - interval.min());
+ OnFrameAcked(QuicFrame(&crypto_frame), QuicTime::Delta::Zero(),
+ QuicTime::Zero());
+ }
return;
}
for (const auto& interval : crypto_bytes_transferred_[ENCRYPTION_INITIAL]) {
