gfe-relnote: In QUIC, send PING when TLP fires and no packet is created. Protected by existing gfe2_reloadable_flag_quic_fix_rto_retransmission3.
PiperOrigin-RevId: 266418710
Change-Id: I1979a67e966adee15a2926f1628ff5175ef5707d
diff --git a/quic/core/quic_connection.cc b/quic/core/quic_connection.cc
index cc99744..257ef29 100644
--- a/quic/core/quic_connection.cc
+++ b/quic/core/quic_connection.cc
@@ -2586,11 +2586,19 @@
if (sent_packet_manager_.fix_rto_retransmission()) {
if (packet_generator_.packet_number() == previous_created_packet_number &&
- (retransmission_mode == QuicSentPacketManager::RTO_MODE ||
+ (retransmission_mode == QuicSentPacketManager::TLP_MODE ||
+ retransmission_mode == QuicSentPacketManager::RTO_MODE ||
retransmission_mode == QuicSentPacketManager::PTO_MODE) &&
!visitor_->WillingAndAbleToWrite()) {
// Send PING if timer fires in RTO or PTO mode but there is no data to
// send.
+ // When TLP fires, either new data or tail loss probe should be sent.
+ // There is corner case where TLP fires after RTO because packets get
+ // acked. Two packets are marked RTO_RETRANSMITTED, but the first packet
+ // is retransmitted as two packets because of packet number length
+ // increases (please see QuicConnectionTest.RtoPacketAsTwo).
+ QUIC_BUG_IF(retransmission_mode == QuicSentPacketManager::TLP_MODE &&
+ stats_.rto_count == 0);
DCHECK_LT(0u, sent_packet_manager_.pending_timer_transmission_count());
visitor_->SendPing();
}
diff --git a/quic/core/quic_connection_test.cc b/quic/core/quic_connection_test.cc
index 9f8127d..ec09686 100644
--- a/quic/core/quic_connection_test.cc
+++ b/quic/core/quic_connection_test.cc
@@ -1283,8 +1283,10 @@
StreamSendingState state,
QuicPacketNumber* last_packet) {
QuicByteCount packet_size;
+ // Save the last packet's size.
EXPECT_CALL(*send_algorithm_, OnPacketSent(_, _, _, _, _))
- .WillOnce(SaveArg<3>(&packet_size));
+ .Times(AnyNumber())
+ .WillRepeatedly(SaveArg<3>(&packet_size));
connection_.SendStreamDataWithString(id, data, offset, state);
if (last_packet != nullptr) {
*last_packet = creator_->packet_number();
@@ -9156,6 +9158,52 @@
connection_close_frames[0].transport_close_frame_type);
}
+// Regression test for b/137401387 and b/138962304.
+TEST_P(QuicConnectionTest, RtoPacketAsTwo) {
+ if (!QuicConnectionPeer::GetSentPacketManager(&connection_)
+ ->fix_rto_retransmission() ||
+ connection_.PtoEnabled()) {
+ return;
+ }
+ connection_.SetMaxTailLossProbes(1);
+ connection_.SetDefaultEncryptionLevel(ENCRYPTION_FORWARD_SECURE);
+ std::string stream_data(3000, 's');
+ // Send packets 1 - 66 and exhaust cwnd.
+ for (size_t i = 0; i < 22; ++i) {
+ // 3 packets for each stream, the first 2 are guaranteed to be full packets.
+ SendStreamDataToPeer(i + 2, stream_data, 0, FIN, nullptr);
+ }
+ CongestionBlockWrites();
+
+ // Fires TLP. Please note, this tail loss probe has 1 byte less stream data
+ // compared to packet 1 because packet number length increases.
+ EXPECT_CALL(*send_algorithm_, OnPacketSent(_, _, QuicPacketNumber(67), _, _));
+ connection_.GetRetransmissionAlarm()->Fire();
+ // Fires RTO. Please note, although packets 2 and 3 *should* be RTOed, but
+ // packet 2 gets RTOed to two packets because packet number length increases.
+ EXPECT_CALL(*send_algorithm_, OnPacketSent(_, _, QuicPacketNumber(68), _, _));
+ EXPECT_CALL(*send_algorithm_, OnPacketSent(_, _, QuicPacketNumber(69), _, _));
+ connection_.GetRetransmissionAlarm()->Fire();
+
+ EXPECT_CALL(visitor_, OnSuccessfulVersionNegotiation(_));
+ // Resets all streams except 2 and ack packets 1 and 2. Now, packet 3 is the
+ // only one containing retransmittable frames.
+ for (size_t i = 1; i < 22; ++i) {
+ notifier_.OnStreamReset(i + 2, QUIC_STREAM_CANCELLED);
+ }
+ EXPECT_CALL(*send_algorithm_, OnCongestionEvent(_, _, _, _, _));
+ QuicAckFrame frame =
+ InitAckFrame({{QuicPacketNumber(1), QuicPacketNumber(3)}});
+ ProcessAckPacket(1, &frame);
+ CongestionUnblockWrites();
+
+ // Fires TLP, verify a PING gets sent because packet 3 is marked
+ // RTO_RETRANSMITTED.
+ EXPECT_CALL(*send_algorithm_, OnPacketSent(_, _, QuicPacketNumber(70), _, _));
+ EXPECT_CALL(visitor_, SendPing()).WillOnce(Invoke([this]() { SendPing(); }));
+ connection_.GetRetransmissionAlarm()->Fire();
+}
+
} // namespace
} // namespace test
} // namespace quic
diff --git a/quic/test_tools/simple_session_notifier.cc b/quic/test_tools/simple_session_notifier.cc
index 16c441c..72a23d7 100644
--- a/quic/test_tools/simple_session_notifier.cc
+++ b/quic/test_tools/simple_session_notifier.cc
@@ -175,6 +175,14 @@
}
}
+void SimpleSessionNotifier::OnStreamReset(QuicStreamId id,
+ QuicRstStreamErrorCode error) {
+ if (error != QUIC_STREAM_NO_ERROR) {
+ // Delete stream to avoid retransmissions.
+ stream_map_.erase(id);
+ }
+}
+
bool SimpleSessionNotifier::WillingToWrite() const {
QUIC_DVLOG(1) << "has_buffered_control_frames: " << HasBufferedControlFrames()
<< " as_lost_control_frames: " << !lost_control_frames_.empty()
diff --git a/quic/test_tools/simple_session_notifier.h b/quic/test_tools/simple_session_notifier.h
index 7424053..b3d5d72 100644
--- a/quic/test_tools/simple_session_notifier.h
+++ b/quic/test_tools/simple_session_notifier.h
@@ -45,6 +45,9 @@
// Called when connection_ becomes writable.
void OnCanWrite();
+ // Called to reset stream.
+ void OnStreamReset(QuicStreamId id, QuicRstStreamErrorCode error);
+
// Returns true if there are 1) unsent control frames and stream data, or 2)
// lost control frames and stream data.
bool WillingToWrite() const;
