gfe-relnote: In QUIC, when RTO fires and there is no packet to be RTOed, let connection send data. Protected by gfe2_reloadable_flag_quic_fix_rto_retransmission.

PiperOrigin-RevId: 258417558
Change-Id: I75267afafee6834f7b6f4cd59d08bdc036c3bd58

quic/core/quic_connection.cc

diff --git a/quic/core/quic_connection.cc b/quic/core/quic_connection.cc
index 1ad2ddb..66b3dd4 100644
--- a/quic/core/quic_connection.cc
+++ b/quic/core/quic_connection.cc
@@ -2422,6 +2422,13 @@
 
 void QuicConnection::OnRetransmissionTimeout() {
   DCHECK(!sent_packet_manager_.unacked_packets().empty());
+  const QuicPacketNumber previous_created_packet_number =
+      packet_generator_.packet_number();
+  const size_t previous_crypto_retransmit_count =
+      stats_.crypto_retransmit_count;
+  const size_t previous_loss_timeout_count = stats_.loss_timeout_count;
+  const size_t previous_tlp_count = stats_.tlp_count;
+  const size_t pervious_rto_count = stats_.rto_count;
   if (close_connection_after_five_rtos_ &&
       sent_packet_manager_.GetConsecutiveRtoCount() >= 4) {
     // Close on the 5th consecutive RTO, so after 4 previous RTOs have occurred.
@@ -2446,6 +2453,29 @@
     WriteIfNotBlocked();
   }
 
+  if (sent_packet_manager_.fix_rto_retransmission()) {
+    // Making sure at least one packet is created when retransmission timer
+    // fires in TLP, RTO or HANDSHAKE mode. It is possible that loss algorithm
+    // invokes timer based loss but the packet does not need to be
+    // retransmitted.
+    QUIC_BUG_IF(stats_.loss_timeout_count == previous_loss_timeout_count &&
+                packet_generator_.packet_number() ==
+                    previous_created_packet_number)
+        << "previous_crypto_retransmit_count: "
+        << previous_crypto_retransmit_count
+        << ", crypto_retransmit_count: " << stats_.crypto_retransmit_count
+        << ", previous_loss_timeout_count: " << previous_loss_timeout_count
+        << ", loss_timeout_count: " << stats_.loss_timeout_count
+        << ", previous_tlp_count: " << previous_tlp_count
+        << ", tlp_count: " << stats_.tlp_count
+        << ", pervious_rto_count: " << pervious_rto_count
+        << ", rto_count: " << stats_.rto_count
+        << ", previous_created_packet_number: "
+        << previous_created_packet_number
+        << ", packet_number: " << packet_generator_.packet_number()
+        << ", session has data to write: " << visitor_->WillingAndAbleToWrite();
+  }
+
   // Ensure the retransmission alarm is always set if there are unacked packets
   // and nothing waiting to be sent.
   // This happens if the loss algorithm invokes a timer based loss, but the

quic/core/quic_connection_test.cc

diff --git a/quic/core/quic_connection_test.cc b/quic/core/quic_connection_test.cc
index f04d142..6b5b943 100644
--- a/quic/core/quic_connection_test.cc
+++ b/quic/core/quic_connection_test.cc
@@ -4065,6 +4065,54 @@
   EXPECT_EQ(QuicPacketNumber(1u), stop_waiting()->least_unacked);
 }
 
+// Regression test of b/133771183.
+TEST_P(QuicConnectionTest, RtoWithNoDataToRetransmit) {
+  if (!connection_.session_decides_what_to_write()) {
+    return;
+  }
+  connection_.SetDefaultEncryptionLevel(ENCRYPTION_FORWARD_SECURE);
+  EXPECT_CALL(visitor_, OnSuccessfulVersionNegotiation(_));
+  connection_.SetMaxTailLossProbes(0);
+
+  SendStreamDataToPeer(3, "foo", 0, NO_FIN, nullptr);
+  // Connection is cwnd limited.
+  CongestionBlockWrites();
+  // Stream gets reset.
+  SendRstStream(3, QUIC_ERROR_PROCESSING_STREAM, 3);
+  // Simulate the retransmission alarm firing.
+  clock_.AdvanceTime(DefaultRetransmissionTime());
+  // RTO fires, but there is no packet to be RTOed.
+  if (GetQuicReloadableFlag(quic_fix_rto_retransmission)) {
+    EXPECT_CALL(*send_algorithm_, OnPacketSent(_, _, _, _, _)).Times(1);
+  } else {
+    EXPECT_CALL(*send_algorithm_, OnPacketSent(_, _, _, _, _)).Times(0);
+  }
+  connection_.GetRetransmissionAlarm()->Fire();
+  if (GetQuicReloadableFlag(quic_fix_rto_retransmission)) {
+    EXPECT_EQ(1u, writer_->rst_stream_frames().size());
+  }
+
+  EXPECT_CALL(visitor_, OnStreamFrame(_)).Times(40);
+  EXPECT_CALL(*send_algorithm_, OnPacketSent(_, _, _, _, _)).Times(20);
+  if (GetQuicReloadableFlag(quic_fix_rto_retransmission)) {
+    EXPECT_CALL(visitor_, WillingAndAbleToWrite())
+        .WillRepeatedly(Return(false));
+  } else {
+    EXPECT_CALL(visitor_, WillingAndAbleToWrite()).WillRepeatedly(Return(true));
+  }
+  if (GetQuicReloadableFlag(quic_fix_rto_retransmission)) {
+    EXPECT_CALL(visitor_, OnAckNeedsRetransmittableFrame()).Times(1);
+  } else {
+    // Since there is a buffered RST_STREAM, no retransmittable frame is bundled
+    // with ACKs.
+    EXPECT_CALL(visitor_, OnAckNeedsRetransmittableFrame()).Times(0);
+  }
+  // Receives packets 1 - 40.
+  for (size_t i = 1; i <= 40; ++i) {
+    ProcessDataPacket(i);
+  }
+}
+
 TEST_P(QuicConnectionTest, RetransmitWithSameEncryptionLevel) {
   use_tagging_decrypter();
 

quic/core/quic_sent_packet_manager.cc

diff --git a/quic/core/quic_sent_packet_manager.cc b/quic/core/quic_sent_packet_manager.cc
index a4aa08e..a93115b 100644
--- a/quic/core/quic_sent_packet_manager.cc
+++ b/quic/core/quic_sent_packet_manager.cc
@@ -120,10 +120,15 @@
       loss_removes_from_inflight_(
           GetQuicReloadableFlag(quic_loss_removes_from_inflight)),
       ignore_tlpr_if_no_pending_stream_data_(
-          GetQuicReloadableFlag(quic_ignore_tlpr_if_no_pending_stream_data)) {
+          GetQuicReloadableFlag(quic_ignore_tlpr_if_no_pending_stream_data)),
+      fix_rto_retransmission_(
+          GetQuicReloadableFlag(quic_fix_rto_retransmission)) {
   if (loss_removes_from_inflight_) {
     QUIC_RELOADABLE_FLAG_COUNT(quic_loss_removes_from_inflight);
   }
+  if (fix_rto_retransmission_) {
+    QUIC_RELOADABLE_FLAG_COUNT(quic_fix_rto_retransmission);
+  }
   SetSendAlgorithm(congestion_control_type);
 }
 
@@ -803,7 +808,7 @@
     if (session_decides_what_to_write()) {
       has_retransmissions = it->state != OUTSTANDING;
     }
-    if (it->in_flight && !has_retransmissions &&
+    if (!fix_rto_retransmission_ && it->in_flight && !has_retransmissions &&
         !unacked_packets_.HasRetransmittableFrames(*it)) {
       // Log only for non-retransmittable data.
       // Retransmittable data is marked as lost during loss detection, and will
@@ -825,6 +830,13 @@
     for (QuicPacketNumber retransmission : retransmissions) {
       MarkForRetransmission(retransmission, RTO_RETRANSMISSION);
     }
+    if (fix_rto_retransmission_ && retransmissions.empty()) {
+      QUIC_BUG_IF(pending_timer_transmission_count_ != 0);
+      // No packets to be RTO retransmitted, raise up a credit to allow
+      // connection to send.
+      QUIC_CODE_COUNT(no_packets_to_be_rto_retransmitted);
+      pending_timer_transmission_count_ = 1;
+    }
   }
 }
 

quic/core/quic_sent_packet_manager.h

diff --git a/quic/core/quic_sent_packet_manager.h b/quic/core/quic_sent_packet_manager.h
index b90d977..4727724 100644
--- a/quic/core/quic_sent_packet_manager.h
+++ b/quic/core/quic_sent_packet_manager.h
@@ -389,6 +389,8 @@
     return ignore_tlpr_if_no_pending_stream_data_;
   }
 
+  bool fix_rto_retransmission() const { return fix_rto_retransmission_; }
+
  private:
   friend class test::QuicConnectionPeer;
   friend class test::QuicSentPacketManagerPeer;
@@ -634,6 +636,9 @@
 
   // Latched value of quic_ignore_tlpr_if_no_pending_stream_data.
   const bool ignore_tlpr_if_no_pending_stream_data_;
+
+  // Latched value of quic_fix_rto_retransmission.
+  const bool fix_rto_retransmission_;
 };
 
 }  // namespace quic

quic/core/quic_sent_packet_manager_test.cc

diff --git a/quic/core/quic_sent_packet_manager_test.cc b/quic/core/quic_sent_packet_manager_test.cc
index 075690e..fb801d0 100644
--- a/quic/core/quic_sent_packet_manager_test.cc
+++ b/quic/core/quic_sent_packet_manager_test.cc
@@ -2868,6 +2868,37 @@
             manager_.OnAckFrameEnd(clock_.Now(), ENCRYPTION_INITIAL));
 }
 
+TEST_P(QuicSentPacketManagerTest, RtoFiresNoPacketToRetransmit) {
+  if (!manager_.session_decides_what_to_write()) {
+    return;
+  }
+  // Send 10 packets.
+  for (size_t i = 1; i <= 10; ++i) {
+    SendDataPacket(i);
+  }
+  EXPECT_CALL(notifier_, RetransmitFrames(_, _))
+      .Times(2)
+      .WillOnce(WithArgs<1>(Invoke(
+          [this](TransmissionType type) { RetransmitDataPacket(11, type); })))
+      .WillOnce(WithArgs<1>(Invoke(
+          [this](TransmissionType type) { RetransmitDataPacket(12, type); })));
+  manager_.OnRetransmissionTimeout();
+  EXPECT_EQ(1u, stats_.rto_count);
+  EXPECT_EQ(0u, manager_.pending_timer_transmission_count());
+
+  // RTO fires again, but there is no packet to be RTO retransmitted.
+  EXPECT_CALL(notifier_, IsFrameOutstanding(_)).WillRepeatedly(Return(false));
+  EXPECT_CALL(notifier_, RetransmitFrames(_, _)).Times(0);
+  manager_.OnRetransmissionTimeout();
+  EXPECT_EQ(2u, stats_.rto_count);
+  if (GetQuicReloadableFlag(quic_fix_rto_retransmission)) {
+    // Verify a credit is raised up.
+    EXPECT_EQ(1u, manager_.pending_timer_transmission_count());
+  } else {
+    EXPECT_EQ(0u, manager_.pending_timer_transmission_count());
+  }
+}
+
 }  // namespace
 }  // namespace test
 }  // namespace quic