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_retransmission2 which replaces gfe2_reloadable_flag_quic_fix_rto_retransmission.
This fix only applies for version > 39.
PiperOrigin-RevId: 262347120
Change-Id: Ic164c2f9e7117c3f9af43f1f1a3e8b369d8a3052
diff --git a/quic/core/quic_connection.cc b/quic/core/quic_connection.cc
index a577bf1..ce60171 100644
--- a/quic/core/quic_connection.cc
+++ b/quic/core/quic_connection.cc
@@ -2469,12 +2469,12 @@
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)
+ // fires in TLP. It is possible that loss algorithm invokes timer based loss
+ // but the packet does not need to be retransmitted. And no packets will be
+ // sent if timer fires in HANDSHAKE or RTO mode but writer is blocked.
+ QUIC_BUG_IF(packet_generator_.packet_number() ==
+ previous_created_packet_number &&
+ stats_.tlp_count != previous_tlp_count)
<< "previous_crypto_retransmit_count: "
<< previous_crypto_retransmit_count
<< ", crypto_retransmit_count: " << stats_.crypto_retransmit_count
@@ -2487,7 +2487,8 @@
<< ", previous_created_packet_number: "
<< previous_created_packet_number
<< ", packet_number: " << packet_generator_.packet_number()
- << ", session has data to write: " << visitor_->WillingAndAbleToWrite();
+ << ", session has data to write: " << visitor_->WillingAndAbleToWrite()
+ << ", writer is blocked: " << writer_->IsWriteBlocked();
}
// Ensure the retransmission alarm is always set if there are unacked packets
diff --git a/quic/core/quic_connection_test.cc b/quic/core/quic_connection_test.cc
index 39d2760..26c70f1 100644
--- a/quic/core/quic_connection_test.cc
+++ b/quic/core/quic_connection_test.cc
@@ -4159,25 +4159,25 @@
// Simulate the retransmission alarm firing.
clock_.AdvanceTime(DefaultRetransmissionTime());
// RTO fires, but there is no packet to be RTOed.
- if (GetQuicReloadableFlag(quic_fix_rto_retransmission)) {
+ if (GetQuicReloadableFlag(quic_fix_rto_retransmission2)) {
EXPECT_CALL(*send_algorithm_, OnPacketSent(_, _, _, _, _)).Times(1);
} else {
EXPECT_CALL(*send_algorithm_, OnPacketSent(_, _, _, _, _)).Times(0);
}
connection_.GetRetransmissionAlarm()->Fire();
- if (GetQuicReloadableFlag(quic_fix_rto_retransmission)) {
+ if (GetQuicReloadableFlag(quic_fix_rto_retransmission2)) {
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)) {
+ if (GetQuicReloadableFlag(quic_fix_rto_retransmission2)) {
EXPECT_CALL(visitor_, WillingAndAbleToWrite())
.WillRepeatedly(Return(false));
} else {
EXPECT_CALL(visitor_, WillingAndAbleToWrite()).WillRepeatedly(Return(true));
}
- if (GetQuicReloadableFlag(quic_fix_rto_retransmission)) {
+ if (GetQuicReloadableFlag(quic_fix_rto_retransmission2)) {
EXPECT_CALL(visitor_, OnAckNeedsRetransmittableFrame()).Times(1);
} else {
// Since there is a buffered RST_STREAM, no retransmittable frame is bundled
@@ -8685,6 +8685,62 @@
EXPECT_TRUE(connection_.connected());
}
+// Regresstion test for b/138962304.
+TEST_P(QuicConnectionTest, RtoAndWriteBlocked) {
+ if (!QuicConnectionPeer::GetSentPacketManager(&connection_)
+ ->fix_rto_retransmission()) {
+ return;
+ }
+ EXPECT_FALSE(connection_.GetRetransmissionAlarm()->IsSet());
+
+ QuicStreamId stream_id = 2;
+ QuicPacketNumber last_data_packet;
+ SendStreamDataToPeer(stream_id, "foo", 0, NO_FIN, &last_data_packet);
+ EXPECT_TRUE(connection_.GetRetransmissionAlarm()->IsSet());
+
+ // Writer gets blocked.
+ writer_->SetWriteBlocked();
+
+ // Cancel the stream.
+ EXPECT_CALL(*send_algorithm_, OnPacketSent(_, _, _, _, _)).Times(0);
+ EXPECT_CALL(visitor_, OnWriteBlocked()).Times(AtLeast(1));
+ SendRstStream(stream_id, QUIC_ERROR_PROCESSING_STREAM, 3);
+
+ // Retransmission timer fires in RTO mode.
+ connection_.GetRetransmissionAlarm()->Fire();
+ // Verify no packets get flushed when writer is blocked.
+ EXPECT_EQ(0u, connection_.NumQueuedPackets());
+}
+
+// Regresstion test for b/138962304.
+TEST_P(QuicConnectionTest, TlpAndWriteBlocked) {
+ if (!QuicConnectionPeer::GetSentPacketManager(&connection_)
+ ->fix_rto_retransmission()) {
+ return;
+ }
+ EXPECT_FALSE(connection_.GetRetransmissionAlarm()->IsSet());
+ connection_.SetMaxTailLossProbes(1);
+
+ QuicStreamId stream_id = 2;
+ QuicPacketNumber last_data_packet;
+ SendStreamDataToPeer(stream_id, "foo", 0, NO_FIN, &last_data_packet);
+ SendStreamDataToPeer(4, "foo", 0, NO_FIN, &last_data_packet);
+ EXPECT_TRUE(connection_.GetRetransmissionAlarm()->IsSet());
+
+ // Writer gets blocked.
+ writer_->SetWriteBlocked();
+
+ // Cancel stream 2.
+ EXPECT_CALL(*send_algorithm_, OnPacketSent(_, _, _, _, _)).Times(0);
+ EXPECT_CALL(visitor_, OnWriteBlocked()).Times(AtLeast(1));
+ SendRstStream(stream_id, QUIC_ERROR_PROCESSING_STREAM, 3);
+
+ // Retransmission timer fires in TLP mode.
+ connection_.GetRetransmissionAlarm()->Fire();
+ // Verify one packets is forced flushed when writer is blocked.
+ EXPECT_EQ(1u, connection_.NumQueuedPackets());
+}
+
} // namespace
} // namespace test
} // namespace quic
diff --git a/quic/core/quic_sent_packet_manager.cc b/quic/core/quic_sent_packet_manager.cc
index 0542edb..43f530e 100644
--- a/quic/core/quic_sent_packet_manager.cc
+++ b/quic/core/quic_sent_packet_manager.cc
@@ -119,14 +119,10 @@
GetQuicReloadableFlag(quic_loss_removes_from_inflight)),
ignore_tlpr_if_no_pending_stream_data_(
GetQuicReloadableFlag(quic_ignore_tlpr_if_no_pending_stream_data)),
- fix_rto_retransmission_(
- GetQuicReloadableFlag(quic_fix_rto_retransmission)) {
+ fix_rto_retransmission_(false) {
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);
}
@@ -1287,6 +1283,16 @@
rtt_stats_.set_initial_rtt(std::max(min_rtt, std::min(max_rtt, rtt)));
}
+void QuicSentPacketManager::SetSessionDecideWhatToWrite(
+ bool session_decides_what_to_write) {
+ if (GetQuicReloadableFlag(quic_fix_rto_retransmission2) &&
+ session_decides_what_to_write) {
+ fix_rto_retransmission_ = true;
+ QUIC_RELOADABLE_FLAG_COUNT(quic_fix_rto_retransmission2);
+ }
+ unacked_packets_.SetSessionDecideWhatToWrite(session_decides_what_to_write);
+}
+
void QuicSentPacketManager::EnableMultiplePacketNumberSpacesSupport() {
unacked_packets_.EnableMultiplePacketNumberSpacesSupport();
}
diff --git a/quic/core/quic_sent_packet_manager.h b/quic/core/quic_sent_packet_manager.h
index 010181c..98eb33e 100644
--- a/quic/core/quic_sent_packet_manager.h
+++ b/quic/core/quic_sent_packet_manager.h
@@ -283,9 +283,7 @@
EncryptionLevel ack_decrypted_level);
// Called to enable/disable letting session decide what to write.
- void SetSessionDecideWhatToWrite(bool session_decides_what_to_write) {
- unacked_packets_.SetSessionDecideWhatToWrite(session_decides_what_to_write);
- }
+ void SetSessionDecideWhatToWrite(bool session_decides_what_to_write);
void EnableMultiplePacketNumberSpacesSupport();
@@ -630,8 +628,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_;
+ // Latched value of quic_fix_rto_retransmission2 and
+ // session_decides_what_to_write.
+ 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 fe34851..e1ab409 100644
--- a/quic/core/quic_sent_packet_manager_test.cc
+++ b/quic/core/quic_sent_packet_manager_test.cc
@@ -2972,7 +2972,7 @@
EXPECT_CALL(notifier_, RetransmitFrames(_, _)).Times(0);
manager_.OnRetransmissionTimeout();
EXPECT_EQ(2u, stats_.rto_count);
- if (GetQuicReloadableFlag(quic_fix_rto_retransmission)) {
+ if (GetQuicReloadableFlag(quic_fix_rto_retransmission2)) {
// Verify a credit is raised up.
EXPECT_EQ(1u, manager_.pending_timer_transmission_count());
} else {
