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 {