Remove DCHECK in QuicPingManager.
This change allows the `retransmittable_on_wire_timeout` to be effectively disabled, which is necessary for conducting specific QUIC experiments.
Currently, if `retransmittable_on_wire_timeout_` is set to 0, Chromium overwrites the value to 200ms in `quic_session_pool.cc`. This behavior prevents us from setting a value greater than keep_alive_timeout_ and thus disabling the retransmittable on wire timeout.
https://source.chromium.org/chromium/chromium/src/+/main:net/quic/quic_session_pool.cc;l=2234-2237;drc=2e7604d8aa7dd54f8f4b60c1a1af66c6fbc427fe
This change modifies the logic to permit `retransmittable_on_wire_timeout_` to be set to a value that will always be greater than the keep_alive_timeout_. As a result, `GetEarliestDeadline()` will consistently return keep_alive_deadline_ as the deadline, allowing us to disable the retransmittable_on_wire_timeout for experimental purposes.
PiperOrigin-RevId: 803221973
diff --git a/quiche/quic/core/quic_connection_test.cc b/quiche/quic/core/quic_connection_test.cc
index ee93bb0..6e819ff 100644
--- a/quiche/quic/core/quic_connection_test.cc
+++ b/quiche/quic/core/quic_connection_test.cc
@@ -8585,6 +8585,55 @@
connection_.GetPingAlarm()->deadline() - clock_.ApproximateNow());
}
+TEST_P(QuicConnectionTest, RetransmittableOnWireTimeoutGreaterThanPingTimeout) {
+ static constexpr QuicTime::Delta kInitialRetransmittableOnWireTimeout =
+ QuicTime::Delta::FromSeconds(300);
+ static constexpr auto kPingTimeout =
+ QuicTime::Delta::FromSeconds(kPingTimeoutSecs);
+
+ connection_.set_initial_retransmittable_on_wire_timeout(
+ kInitialRetransmittableOnWireTimeout);
+
+ EXPECT_TRUE(connection_.connected());
+ EXPECT_CALL(visitor_, ShouldKeepConnectionAlive())
+ .WillRepeatedly(Return(true));
+ // Send a data packet to the peer
+ EXPECT_FALSE(connection_.GetPingAlarm()->IsSet());
+ connection_.SendStreamDataWithString(1, "data", 0, NO_FIN);
+ EXPECT_TRUE(connection_.sent_packet_manager().HasInFlightPackets());
+ // The ping alarm is set for the ping timeout, not the
+ // retransmittable_on_wire_timeout.
+ EXPECT_TRUE(connection_.GetPingAlarm()->IsSet());
+ EXPECT_EQ(kPingTimeout,
+ connection_.GetPingAlarm()->deadline() - clock_.ApproximateNow());
+
+ EXPECT_CALL(visitor_, OnSuccessfulVersionNegotiation(_)).Times(AnyNumber());
+ EXPECT_CALL(*send_algorithm_, OnCongestionEvent(true, _, _, _, _, _, _))
+ .Times(AnyNumber());
+
+ // Receive an ACK of the first packet. Verify there are no in flight packets.
+ // This should still set the ping alarm with kPingTimeout since
+ // retransmittable_on_wire_timeout is greater than kPingTimeout.
+ QUICHE_DCHECK_GT(kInitialRetransmittableOnWireTimeout, kPingTimeout);
+ {
+ QuicPacketNumber ack_num = creator_->packet_number();
+ QuicAckFrame frame = InitAckFrame(
+ {{QuicPacketNumber(ack_num), QuicPacketNumber(ack_num + 1)}});
+ ProcessAckPacket(&frame);
+ EXPECT_FALSE(connection_.sent_packet_manager().HasInFlightPackets());
+ EXPECT_TRUE(connection_.GetPingAlarm()->IsSet());
+ EXPECT_EQ(kPingTimeout,
+ connection_.GetPingAlarm()->deadline() - clock_.ApproximateNow());
+ }
+ // Simulate the alarm firing and check that a PING is sent.
+ writer_->Reset();
+ clock_.AdvanceTime(kPingTimeout);
+ connection_.GetPingAlarm()->Fire();
+
+ // The ping alarm is set with default ping timeout.
+ EXPECT_TRUE(connection_.GetPingAlarm()->IsSet());
+}
+
// Make sure when enabled, the retransmittable on wire timeout is based on the
// PTO.
TEST_P(QuicConnectionTest, PtoBasedRetransmittableOnWireTimeout) {
diff --git a/quiche/quic/core/quic_ping_manager.cc b/quiche/quic/core/quic_ping_manager.cc
index 52a5cd4..7797a35 100644
--- a/quiche/quic/core/quic_ping_manager.cc
+++ b/quiche/quic/core/quic_ping_manager.cc
@@ -115,8 +115,6 @@
static_cast<int>(num_ptos_for_retransmittable_on_wire_timeout_) *
pto_delay;
} else {
- QUICHE_DCHECK_LT(initial_retransmittable_on_wire_timeout_,
- keep_alive_timeout_);
retransmittable_on_wire_timeout = initial_retransmittable_on_wire_timeout_;
}
diff --git a/quiche/quic/core/quic_ping_manager_test.cc b/quiche/quic/core/quic_ping_manager_test.cc
index 0ab3331..7db8dcd 100644
--- a/quiche/quic/core/quic_ping_manager_test.cc
+++ b/quiche/quic/core/quic_ping_manager_test.cc
@@ -534,6 +534,39 @@
}
}
+TEST_F(QuicPingManagerTest,
+ SetAlarmAgnosticToRelativeSizeOfKeepAliveTimeoutVsROWTimeout) {
+ struct TestCase {
+ QuicTime::Delta retransmittable_on_wire_timeout;
+ QuicTime::Delta keep_alive_timeout;
+ };
+ static constexpr TestCase kTestCases[] = {
+ {QuicTime::Delta::FromSeconds(3), QuicTime::Delta::FromSeconds(4)},
+ {QuicTime::Delta::FromSeconds(6), QuicTime::Delta::FromSeconds(5)},
+ {QuicTime::Delta::FromSeconds(7), QuicTime::Delta::FromSeconds(7)}};
+ for (const auto& test_case : kTestCases) {
+ SCOPED_TRACE(testing::Message()
+ << "retransmittable_on_wire_timeout: "
+ << test_case.retransmittable_on_wire_timeout
+ << ", keep_alive_timeout: " << test_case.keep_alive_timeout);
+ manager_.Stop();
+ alarm_->Cancel();
+ EXPECT_FALSE(alarm_->IsSet());
+
+ manager_.set_initial_retransmittable_on_wire_timeout(
+ test_case.retransmittable_on_wire_timeout);
+ manager_.set_keep_alive_timeout(test_case.keep_alive_timeout);
+
+ manager_.SetAlarm(clock_.ApproximateNow(), kShouldKeepAlive,
+ /*has_in_flight_packets=*/false, kPtoDelay);
+ EXPECT_TRUE(alarm_->IsSet());
+
+ EXPECT_EQ(alarm_->deadline() - clock_.ApproximateNow(),
+ std::min(test_case.retransmittable_on_wire_timeout,
+ test_case.keep_alive_timeout));
+ }
+}
+
} // namespace
} // namespace test
