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
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
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();
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;
+ }
}
}
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
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
