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 {
