gfe-relnote: ignore TLPR for retransmission delay in TLP mode when sending pings from the ping alarm. Flag protected by quic_ignore_tlpr_if_stream_not_waiting_ack.
This change will help mitigate the issue that PING packets sent by retransmittable-on-wire are retransmitted too aggressively in TLPR.
PiperOrigin-RevId: 242968845
Change-Id: I7e346bc309cbb52411fa3a1f0ed11615f16a61bf
diff --git a/quic/core/quic_connection.cc b/quic/core/quic_connection.cc
index 311651c..8546076 100644
--- a/quic/core/quic_connection.cc
+++ b/quic/core/quic_connection.cc
@@ -2745,7 +2745,17 @@
void QuicConnection::OnPingTimeout() {
if (!retransmission_alarm_->IsSet()) {
+ bool enable_half_rtt_tail_loss_probe =
+ sent_packet_manager_.enable_half_rtt_tail_loss_probe();
+ if (enable_half_rtt_tail_loss_probe &&
+ GetQuicReloadableFlag(quic_ignore_tlpr_if_sending_ping)) {
+ sent_packet_manager_.set_enable_half_rtt_tail_loss_probe(false);
+ }
visitor_->SendPing();
+ if (enable_half_rtt_tail_loss_probe &&
+ GetQuicReloadableFlag(quic_ignore_tlpr_if_sending_ping)) {
+ sent_packet_manager_.set_enable_half_rtt_tail_loss_probe(true);
+ }
}
}
diff --git a/quic/core/quic_connection_test.cc b/quic/core/quic_connection_test.cc
index 7345246..cd53edf 100644
--- a/quic/core/quic_connection_test.cc
+++ b/quic/core/quic_connection_test.cc
@@ -1201,6 +1201,14 @@
connection_.OnStreamReset(id, error);
}
+ void SendPing() {
+ if (connection_.session_decides_what_to_write()) {
+ notifier_.WriteOrBufferPing();
+ } else {
+ connection_.SendControlFrame(QuicFrame(QuicPingFrame(1)));
+ }
+ }
+
void ProcessAckPacket(uint64_t packet_number, QuicAckFrame* frame) {
if (packet_number > 1) {
QuicPacketCreatorPeer::SetPacketNumber(&peer_creator_, packet_number - 1);
@@ -3864,6 +3872,164 @@
EXPECT_EQ(QuicPacketNumber(1u), stop_waiting()->least_unacked);
}
+TEST_P(QuicConnectionTest, TailLossProbeDelayForStreamDataInTLPR) {
+ if (connection_.SupportsMultiplePacketNumberSpaces()) {
+ return;
+ }
+ // Set TLPR from QuicConfig.
+ EXPECT_CALL(*send_algorithm_, SetFromConfig(_, _));
+ QuicConfig config;
+ QuicTagVector options;
+ options.push_back(kTLPR);
+ config.SetConnectionOptionsToSend(options);
+ connection_.SetFromConfig(config);
+ connection_.SetMaxTailLossProbes(1);
+
+ SendStreamDataToPeer(3, "foo", 0, NO_FIN, nullptr);
+ EXPECT_EQ(QuicPacketNumber(1u), stop_waiting()->least_unacked);
+
+ QuicTime retransmission_time =
+ connection_.GetRetransmissionAlarm()->deadline();
+ EXPECT_NE(QuicTime::Zero(), retransmission_time);
+ QuicTime::Delta expected_tlp_delay =
+ 0.5 * manager_->GetRttStats()->SmoothedOrInitialRtt();
+ EXPECT_EQ(expected_tlp_delay, retransmission_time - clock_.Now());
+
+ EXPECT_EQ(QuicPacketNumber(1u), writer_->header().packet_number);
+ // Simulate firing of the retransmission alarm and retransmit the packet.
+ EXPECT_CALL(*send_algorithm_, OnPacketSent(_, _, QuicPacketNumber(2), _, _));
+ clock_.AdvanceTime(retransmission_time - clock_.Now());
+ connection_.GetRetransmissionAlarm()->Fire();
+ EXPECT_EQ(QuicPacketNumber(2u), writer_->header().packet_number);
+
+ // We do not raise the high water mark yet.
+ EXPECT_EQ(QuicPacketNumber(1u), stop_waiting()->least_unacked);
+}
+
+TEST_P(QuicConnectionTest, TailLossProbeDelayForNonStreamDataInTLPR) {
+ if (connection_.SupportsMultiplePacketNumberSpaces()) {
+ return;
+ }
+ // Set TLPR from QuicConfig.
+ EXPECT_CALL(*send_algorithm_, SetFromConfig(_, _));
+ QuicConfig config;
+ QuicTagVector options;
+ options.push_back(kTLPR);
+ config.SetConnectionOptionsToSend(options);
+ connection_.SetFromConfig(config);
+ connection_.SetMaxTailLossProbes(1);
+
+ // Sets retransmittable on wire.
+ const QuicTime::Delta retransmittable_on_wire_timeout =
+ QuicTime::Delta::FromMilliseconds(50);
+ connection_.set_retransmittable_on_wire_timeout(
+ retransmittable_on_wire_timeout);
+
+ EXPECT_TRUE(connection_.connected());
+ EXPECT_CALL(visitor_, ShouldKeepConnectionAlive())
+ .WillRepeatedly(Return(true));
+ EXPECT_FALSE(connection_.GetPathDegradingAlarm()->IsSet());
+ EXPECT_FALSE(connection_.IsPathDegrading());
+ EXPECT_FALSE(connection_.GetPingAlarm()->IsSet());
+
+ const char data[] = "data";
+ size_t data_size = strlen(data);
+ QuicStreamOffset offset = 0;
+
+ // Send a data packet.
+ connection_.SendStreamDataWithString(1, data, offset, NO_FIN);
+ offset += data_size;
+
+ // Path degrading alarm should be set when there is a retransmittable packet
+ // on the wire.
+ EXPECT_TRUE(connection_.GetPathDegradingAlarm()->IsSet());
+
+ // Verify the path degrading delay.
+ // First TLP with stream data.
+ QuicTime::Delta srtt = manager_->GetRttStats()->SmoothedOrInitialRtt();
+ QuicTime::Delta expected_delay = 0.5 * srtt;
+ // Add 1st RTO.
+ QuicTime::Delta retransmission_delay =
+ QuicTime::Delta::FromMilliseconds(kDefaultRetransmissionTimeMs);
+ expected_delay = expected_delay + retransmission_delay;
+ // Add 2nd RTO.
+ expected_delay = expected_delay + retransmission_delay * 2;
+ EXPECT_EQ(expected_delay,
+ QuicConnectionPeer::GetSentPacketManager(&connection_)
+ ->GetPathDegradingDelay());
+ ASSERT_TRUE(connection_.sent_packet_manager().HasInFlightPackets());
+
+ // The ping alarm is set for the ping timeout, not the shorter
+ // retransmittable_on_wire_timeout.
+ EXPECT_TRUE(connection_.GetPingAlarm()->IsSet());
+ EXPECT_EQ(QuicTime::Delta::FromSeconds(kPingTimeoutSecs),
+ connection_.GetPingAlarm()->deadline() - clock_.ApproximateNow());
+
+ // Receive an ACK for the data packet.
+ clock_.AdvanceTime(QuicTime::Delta::FromMilliseconds(5));
+ EXPECT_CALL(visitor_, OnSuccessfulVersionNegotiation(_));
+ EXPECT_CALL(*send_algorithm_, OnCongestionEvent(true, _, _, _, _));
+ QuicAckFrame frame =
+ InitAckFrame({{QuicPacketNumber(1), QuicPacketNumber(2)}});
+ ProcessAckPacket(&frame);
+
+ // Path degrading alarm should be cancelled as there is no more
+ // reretransmittable packets on the wire.
+ EXPECT_FALSE(connection_.GetPathDegradingAlarm()->IsSet());
+ // The ping alarm should be set to the retransmittable_on_wire_timeout.
+ EXPECT_TRUE(connection_.GetPingAlarm()->IsSet());
+ EXPECT_EQ(retransmittable_on_wire_timeout,
+ connection_.GetPingAlarm()->deadline() - clock_.ApproximateNow());
+
+ // Simulate firing of the retransmittable on wire and send a PING.
+ EXPECT_CALL(visitor_, SendPing()).WillOnce(Invoke([this]() { SendPing(); }));
+ clock_.AdvanceTime(retransmittable_on_wire_timeout);
+ connection_.GetPingAlarm()->Fire();
+
+ // The retransmission alarm and the path degrading alarm should be set as
+ // there is a retransmittable packet (PING) on the wire,
+ EXPECT_TRUE(connection_.GetRetransmissionAlarm()->IsSet());
+ EXPECT_TRUE(connection_.GetPathDegradingAlarm()->IsSet());
+
+ // Verify the retransmission delay.
+ QuicTime::Delta min_rto_timeout =
+ QuicTime::Delta::FromMilliseconds(kMinRetransmissionTimeMs);
+ srtt = manager_->GetRttStats()->SmoothedOrInitialRtt();
+ if (GetQuicReloadableFlag(quic_ignore_tlpr_if_sending_ping)) {
+ // First TLP without unacked stream data will no longer use TLPR.
+ expected_delay = std::max(2 * srtt, 1.5 * srtt + 0.5 * min_rto_timeout);
+ } else {
+ expected_delay =
+ std::max(QuicTime::Delta::FromMilliseconds(kMinTailLossProbeTimeoutMs),
+ srtt * 0.5);
+ }
+ EXPECT_EQ(expected_delay,
+ connection_.GetRetransmissionAlarm()->deadline() - clock_.Now());
+
+ // Verify the path degrading delay.
+ // Path degrading delay will count TLPR for the tail loss probe delay.
+ expected_delay =
+ std::max(QuicTime::Delta::FromMilliseconds(kMinTailLossProbeTimeoutMs),
+ srtt * 0.5);
+ // Add 1st RTO.
+ retransmission_delay =
+ std::max(manager_->GetRttStats()->smoothed_rtt() +
+ 4 * manager_->GetRttStats()->mean_deviation(),
+ min_rto_timeout);
+ expected_delay = expected_delay + retransmission_delay;
+ // Add 2nd RTO.
+ expected_delay = expected_delay + retransmission_delay * 2;
+ EXPECT_EQ(expected_delay,
+ QuicConnectionPeer::GetSentPacketManager(&connection_)
+ ->GetPathDegradingDelay());
+
+ // The ping alarm is set for the ping timeout, not the shorter
+ // retransmittable_on_wire_timeout.
+ EXPECT_TRUE(connection_.GetPingAlarm()->IsSet());
+ EXPECT_EQ(QuicTime::Delta::FromSeconds(kPingTimeoutSecs),
+ connection_.GetPingAlarm()->deadline() - clock_.ApproximateNow());
+}
+
TEST_P(QuicConnectionTest, RTO) {
if (connection_.SupportsMultiplePacketNumberSpaces()) {
return;
@@ -4435,9 +4601,7 @@
writer_->Reset();
clock_.AdvanceTime(QuicTime::Delta::FromSeconds(15));
- EXPECT_CALL(visitor_, SendPing()).WillOnce(Invoke([this]() {
- connection_.SendControlFrame(QuicFrame(QuicPingFrame(1)));
- }));
+ EXPECT_CALL(visitor_, SendPing()).WillOnce(Invoke([this]() { SendPing(); }));
connection_.GetPingAlarm()->Fire();
EXPECT_EQ(1u, writer_->frame_count());
ASSERT_EQ(1u, writer_->ping_frames().size());
diff --git a/quic/core/quic_sent_packet_manager.h b/quic/core/quic_sent_packet_manager.h
index c892126..039ee59 100644
--- a/quic/core/quic_sent_packet_manager.h
+++ b/quic/core/quic_sent_packet_manager.h
@@ -355,6 +355,15 @@
delayed_ack_time_ = delayed_ack_time;
}
+ bool enable_half_rtt_tail_loss_probe() const {
+ return enable_half_rtt_tail_loss_probe_;
+ }
+
+ void set_enable_half_rtt_tail_loss_probe(
+ bool enable_half_rtt_tail_loss_probe) {
+ enable_half_rtt_tail_loss_probe_ = enable_half_rtt_tail_loss_probe;
+ }
+
const QuicUnackedPacketMap& unacked_packets() const {
return unacked_packets_;
}
diff --git a/quic/test_tools/simple_session_notifier.cc b/quic/test_tools/simple_session_notifier.cc
index 7a76194..643821a 100644
--- a/quic/test_tools/simple_session_notifier.cc
+++ b/quic/test_tools/simple_session_notifier.cc
@@ -112,6 +112,19 @@
WriteBufferedControlFrames();
}
+void SimpleSessionNotifier::WriteOrBufferPing() {
+ QUIC_DVLOG(1) << "Writing PING_FRAME";
+ const bool had_buffered_data =
+ HasBufferedStreamData() || HasBufferedControlFrames();
+ control_frames_.emplace_back(
+ (QuicFrame(QuicPingFrame(++last_control_frame_id_))));
+ if (had_buffered_data) {
+ QUIC_DLOG(WARNING) << "Connection is write blocked";
+ return;
+ }
+ WriteBufferedControlFrames();
+}
+
void SimpleSessionNotifier::NeuterUnencryptedData() {
for (const auto& interval : crypto_bytes_transferred_[ENCRYPTION_INITIAL]) {
// TODO(nharper): Handle CRYPTO frame case.
diff --git a/quic/test_tools/simple_session_notifier.h b/quic/test_tools/simple_session_notifier.h
index f7982d2..17616b7 100644
--- a/quic/test_tools/simple_session_notifier.h
+++ b/quic/test_tools/simple_session_notifier.h
@@ -31,6 +31,8 @@
void WriteOrBufferRstStream(QuicStreamId id,
QuicRstStreamErrorCode error,
QuicStreamOffset bytes_written);
+ // Tries to write PING.
+ void WriteOrBufferPing();
// Tries to write CRYPTO data and returns the number of bytes written.
size_t WriteCryptoData(EncryptionLevel level,
diff --git a/quic/test_tools/simple_session_notifier_test.cc b/quic/test_tools/simple_session_notifier_test.cc
index 136544b..53712fd 100644
--- a/quic/test_tools/simple_session_notifier_test.cc
+++ b/quic/test_tools/simple_session_notifier_test.cc
@@ -100,6 +100,31 @@
EXPECT_FALSE(notifier_.StreamIsWaitingForAcks(5));
}
+TEST_F(SimpleSessionNotifierTest, WriteOrBufferPing) {
+ InSequence s;
+ // Write ping when connection is not write blocked.
+ EXPECT_CALL(connection_, SendControlFrame(_))
+ .WillRepeatedly(
+ Invoke(this, &SimpleSessionNotifierTest::ControlFrameConsumed));
+ notifier_.WriteOrBufferPing();
+ EXPECT_EQ(0u, notifier_.StreamBytesToSend());
+ EXPECT_FALSE(notifier_.WillingToWrite());
+
+ // Write stream data and cause the connection to be write blocked.
+ EXPECT_CALL(connection_, SendStreamData(3, 1024, 0, NO_FIN))
+ .WillOnce(Return(QuicConsumedData(1024, false)));
+ notifier_.WriteOrBufferData(3, 1024, NO_FIN);
+ EXPECT_EQ(0u, notifier_.StreamBytesToSend());
+ EXPECT_CALL(connection_, SendStreamData(5, 512, 0, NO_FIN))
+ .WillOnce(Return(QuicConsumedData(256, false)));
+ notifier_.WriteOrBufferData(5, 512, NO_FIN);
+ EXPECT_TRUE(notifier_.WillingToWrite());
+
+ // Connection is blocked.
+ EXPECT_CALL(connection_, SendControlFrame(_)).Times(0);
+ notifier_.WriteOrBufferPing();
+}
+
TEST_F(SimpleSessionNotifierTest, NeuterUnencryptedData) {
InSequence s;
// Send crypto data [0, 1024) in ENCRYPTION_INITIAL.
