gfe-relnote: In QUIC, schedule RTO whenever there is bytes in flight (rather than pending retransmittable frames). When timer fires in RTO_MODE and there is no data to send, force to send PING. Protected by gfe2_reloadable_flag_quic_fix_rto_retransmission3 which replaces gfe2_reloadable_flag_quic_fix_rto_retransmission2. PiperOrigin-RevId: 263450880 Change-Id: Iccf125ee87c59ce49d53edc38c6460f120008a17
diff --git a/quic/core/quic_connection.cc b/quic/core/quic_connection.cc index ce60171..581709b 100644 --- a/quic/core/quic_connection.cc +++ b/quic/core/quic_connection.cc
@@ -2451,7 +2451,8 @@ return; } - sent_packet_manager_.OnRetransmissionTimeout(); + const auto retransmission_mode = + sent_packet_manager_.OnRetransmissionTimeout(); WriteIfNotBlocked(); // A write failure can result in the connection being closed, don't attempt to @@ -2468,27 +2469,38 @@ } if (sent_packet_manager_.fix_rto_retransmission()) { - // Making sure at least one packet is created when retransmission timer - // 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 - << ", 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() - << ", writer is blocked: " << writer_->IsWriteBlocked(); + if (packet_generator_.packet_number() == previous_created_packet_number && + retransmission_mode == QuicSentPacketManager::RTO_MODE && + !visitor_->WillingAndAbleToWrite()) { + // Send PING if timer fires in RTO mode but there is no data to send. + DCHECK_LT(0u, sent_packet_manager_.pending_timer_transmission_count()); + visitor_->SendPing(); + } + if (retransmission_mode != QuicSentPacketManager::LOSS_MODE && + retransmission_mode != QuicSentPacketManager::HANDSHAKE_MODE) { + // When timer fires in TLP or RTO mode, ensure 1) at least one packet is + // created, or there is data to send and available credit (such that + // packets will be sent eventually). + QUIC_BUG_IF( + packet_generator_.packet_number() == previous_created_packet_number && + (!visitor_->WillingAndAbleToWrite() || + sent_packet_manager_.pending_timer_transmission_count() == 0u)) + << "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() + << ", 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 26c70f1..f90a9db 100644 --- a/quic/core/quic_connection_test.cc +++ b/quic/core/quic_connection_test.cc
@@ -3452,7 +3452,12 @@ // Ensure that the data is still in flight, but the retransmission alarm is no // longer set. EXPECT_GT(manager_->GetBytesInFlight(), 0u); - EXPECT_FALSE(connection_.GetRetransmissionAlarm()->IsSet()); + if (QuicConnectionPeer::GetSentPacketManager(&connection_) + ->fix_rto_retransmission()) { + EXPECT_TRUE(connection_.GetRetransmissionAlarm()->IsSet()); + } else { + EXPECT_FALSE(connection_.GetRetransmissionAlarm()->IsSet()); + } } TEST_P(QuicConnectionTest, RetransmitForQuicRstStreamNoErrorOnRTO) { @@ -3695,7 +3700,12 @@ writer_->SetWritable(); connection_.OnCanWrite(); - EXPECT_FALSE(connection_.GetRetransmissionAlarm()->IsSet()); + if (QuicConnectionPeer::GetSentPacketManager(&connection_) + ->fix_rto_retransmission()) { + EXPECT_TRUE(connection_.GetRetransmissionAlarm()->IsSet()); + } else { + EXPECT_FALSE(connection_.GetRetransmissionAlarm()->IsSet()); + } EXPECT_FALSE(QuicConnectionPeer::HasRetransmittableFrames(&connection_, 2)); } @@ -4159,25 +4169,25 @@ // Simulate the retransmission alarm firing. clock_.AdvanceTime(DefaultRetransmissionTime()); // RTO fires, but there is no packet to be RTOed. - if (GetQuicReloadableFlag(quic_fix_rto_retransmission2)) { + if (GetQuicReloadableFlag(quic_fix_rto_retransmission3)) { EXPECT_CALL(*send_algorithm_, OnPacketSent(_, _, _, _, _)).Times(1); } else { EXPECT_CALL(*send_algorithm_, OnPacketSent(_, _, _, _, _)).Times(0); } connection_.GetRetransmissionAlarm()->Fire(); - if (GetQuicReloadableFlag(quic_fix_rto_retransmission2)) { + if (GetQuicReloadableFlag(quic_fix_rto_retransmission3)) { 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_retransmission2)) { + if (GetQuicReloadableFlag(quic_fix_rto_retransmission3)) { EXPECT_CALL(visitor_, WillingAndAbleToWrite()) .WillRepeatedly(Return(false)); } else { EXPECT_CALL(visitor_, WillingAndAbleToWrite()).WillRepeatedly(Return(true)); } - if (GetQuicReloadableFlag(quic_fix_rto_retransmission2)) { + if (GetQuicReloadableFlag(quic_fix_rto_retransmission3)) { EXPECT_CALL(visitor_, OnAckNeedsRetransmittableFrame()).Times(1); } else { // Since there is a buffered RST_STREAM, no retransmittable frame is bundled @@ -8704,6 +8714,9 @@ // Cancel the stream. EXPECT_CALL(*send_algorithm_, OnPacketSent(_, _, _, _, _)).Times(0); EXPECT_CALL(visitor_, OnWriteBlocked()).Times(AtLeast(1)); + EXPECT_CALL(visitor_, WillingAndAbleToWrite()) + .WillRepeatedly( + Invoke(¬ifier_, &SimpleSessionNotifier::WillingToWrite)); SendRstStream(stream_id, QUIC_ERROR_PROCESSING_STREAM, 3); // Retransmission timer fires in RTO mode. @@ -8741,6 +8754,46 @@ EXPECT_EQ(1u, connection_.NumQueuedPackets()); } +// Regresstion test for b/139375344. +TEST_P(QuicConnectionTest, RtoForcesSendingPing) { + if (!QuicConnectionPeer::GetSentPacketManager(&connection_) + ->fix_rto_retransmission()) { + return; + } + EXPECT_CALL(visitor_, OnSuccessfulVersionNegotiation(_)); + connection_.SetMaxTailLossProbes(2); + EXPECT_EQ(0u, connection_.GetStats().tlp_count); + EXPECT_EQ(0u, connection_.GetStats().rto_count); + + SendStreamDataToPeer(2, "foo", 0, NO_FIN, nullptr); + QuicTime retransmission_time = + connection_.GetRetransmissionAlarm()->deadline(); + EXPECT_NE(QuicTime::Zero(), retransmission_time); + // TLP fires. + EXPECT_CALL(*send_algorithm_, OnPacketSent(_, _, QuicPacketNumber(2), _, _)); + clock_.AdvanceTime(retransmission_time - clock_.Now()); + connection_.GetRetransmissionAlarm()->Fire(); + EXPECT_EQ(1u, connection_.GetStats().tlp_count); + EXPECT_EQ(0u, connection_.GetStats().rto_count); + EXPECT_TRUE(connection_.GetRetransmissionAlarm()->IsSet()); + + // Packet 1 gets acked. + QuicAckFrame frame = InitAckFrame(1); + EXPECT_CALL(*send_algorithm_, OnCongestionEvent(_, _, _, _, _)); + ProcessAckPacket(1, &frame); + EXPECT_TRUE(connection_.GetRetransmissionAlarm()->IsSet()); + retransmission_time = connection_.GetRetransmissionAlarm()->deadline(); + + // RTO fires, verify a PING packet gets sent because there is no data to send. + EXPECT_CALL(*send_algorithm_, OnPacketSent(_, _, QuicPacketNumber(3), _, _)); + EXPECT_CALL(visitor_, SendPing()).WillOnce(Invoke([this]() { SendPing(); })); + clock_.AdvanceTime(retransmission_time - clock_.Now()); + connection_.GetRetransmissionAlarm()->Fire(); + EXPECT_EQ(1u, connection_.GetStats().tlp_count); + EXPECT_EQ(1u, connection_.GetStats().rto_count); + EXPECT_EQ(1u, writer_->ping_frames().size()); +} + } // namespace } // namespace test } // namespace quic
diff --git a/quic/core/quic_sent_packet_manager.cc b/quic/core/quic_sent_packet_manager.cc index 43f530e..54fbdfa 100644 --- a/quic/core/quic_sent_packet_manager.cc +++ b/quic/core/quic_sent_packet_manager.cc
@@ -708,7 +708,8 @@ return in_flight; } -void QuicSentPacketManager::OnRetransmissionTimeout() { +QuicSentPacketManager::RetransmissionTimeoutMode +QuicSentPacketManager::OnRetransmissionTimeout() { DCHECK(unacked_packets_.HasInFlightPackets()); DCHECK_EQ(0u, pending_timer_transmission_count_); // Handshake retransmission, timer based loss detection, TLP, and RTO are @@ -720,14 +721,14 @@ case HANDSHAKE_MODE: ++stats_->crypto_retransmit_count; RetransmitCryptoPackets(); - return; + return HANDSHAKE_MODE; case LOSS_MODE: { ++stats_->loss_timeout_count; QuicByteCount prior_in_flight = unacked_packets_.bytes_in_flight(); const QuicTime now = clock_->Now(); InvokeLossDetection(now); MaybeInvokeCongestionEvent(false, prior_in_flight, now); - return; + return LOSS_MODE; } case TLP_MODE: ++stats_->tlp_count; @@ -735,11 +736,11 @@ pending_timer_transmission_count_ = 1; // TLPs prefer sending new data instead of retransmitting data, so // give the connection a chance to write before completing the TLP. - return; + return TLP_MODE; case RTO_MODE: ++stats_->rto_count; RetransmitRtoPackets(); - return; + return RTO_MODE; } } @@ -950,7 +951,8 @@ pending_timer_transmission_count_ > 0) { return QuicTime::Zero(); } - if (!unacked_packets_.HasUnackedRetransmittableFrames()) { + if (!fix_rto_retransmission_ && + !unacked_packets_.HasUnackedRetransmittableFrames()) { return QuicTime::Zero(); } switch (GetRetransmissionMode()) { @@ -1285,10 +1287,10 @@ void QuicSentPacketManager::SetSessionDecideWhatToWrite( bool session_decides_what_to_write) { - if (GetQuicReloadableFlag(quic_fix_rto_retransmission2) && + if (GetQuicReloadableFlag(quic_fix_rto_retransmission3) && session_decides_what_to_write) { fix_rto_retransmission_ = true; - QUIC_RELOADABLE_FLAG_COUNT(quic_fix_rto_retransmission2); + QUIC_RELOADABLE_FLAG_COUNT(quic_fix_rto_retransmission3); } unacked_packets_.SetSessionDecideWhatToWrite(session_decides_what_to_write); }
diff --git a/quic/core/quic_sent_packet_manager.h b/quic/core/quic_sent_packet_manager.h index 98eb33e..1373646 100644 --- a/quic/core/quic_sent_packet_manager.h +++ b/quic/core/quic_sent_packet_manager.h
@@ -91,6 +91,20 @@ virtual void OnPathMtuIncreased(QuicPacketLength packet_size) = 0; }; + // The retransmission timer is a single timer which switches modes depending + // upon connection state. + enum RetransmissionTimeoutMode { + // A conventional TCP style RTO. + RTO_MODE, + // A tail loss probe. By default, QUIC sends up to two before RTOing. + TLP_MODE, + // Retransmission of handshake packets prior to handshake completion. + HANDSHAKE_MODE, + // Re-invoke the loss detection when a packet is not acked before the + // loss detection algorithm expects. + LOSS_MODE, + }; + QuicSentPacketManager(Perspective perspective, const QuicClock* clock, QuicRandom* random, @@ -183,8 +197,9 @@ TransmissionType transmission_type, HasRetransmittableData has_retransmittable_data); - // Called when the retransmission timer expires. - void OnRetransmissionTimeout(); + // Called when the retransmission timer expires and returns the retransmission + // mode. + RetransmissionTimeoutMode OnRetransmissionTimeout(); // Calculate the time until we can send the next packet to the wire. // Note 1: When kUnknownWaitTime is returned, there is no need to poll @@ -389,20 +404,6 @@ friend class test::QuicConnectionPeer; friend class test::QuicSentPacketManagerPeer; - // The retransmission timer is a single timer which switches modes depending - // upon connection state. - enum RetransmissionTimeoutMode { - // A conventional TCP style RTO. - RTO_MODE, - // A tail loss probe. By default, QUIC sends up to two before RTOing. - TLP_MODE, - // Retransmission of handshake packets prior to handshake completion. - HANDSHAKE_MODE, - // Re-invoke the loss detection when a packet is not acked before the - // loss detection algorithm expects. - LOSS_MODE, - }; - typedef QuicLinkedHashMap<QuicPacketNumber, TransmissionType, QuicPacketNumberHash> @@ -628,7 +629,7 @@ // 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_retransmission2 and + // Latched value of quic_fix_rto_retransmission3 and // session_decides_what_to_write. bool fix_rto_retransmission_; };
diff --git a/quic/core/quic_sent_packet_manager_test.cc b/quic/core/quic_sent_packet_manager_test.cc index e1ab409..56c3d8a 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_retransmission2)) { + if (GetQuicReloadableFlag(quic_fix_rto_retransmission3)) { // Verify a credit is raised up. EXPECT_EQ(1u, manager_.pending_timer_transmission_count()); } else {