Postpone removing handshake timeout from transport params getting negotiated to handshake complete.
Please note, on client side, handshake complete indicates client receives server first flight and can send client finished. On the server side, handshake complete indicates client finished has been received.
Connection option FTOE is used for experiment purpose.
Credit to Stanislav Slusny from Akamai, who reported this issue.
Protected by FLAGS_quic_reloadable_flag_quic_fix_timeouts.
PiperOrigin-RevId: 677553203
diff --git a/quiche/quic/core/crypto/crypto_protocol.h b/quiche/quic/core/crypto/crypto_protocol.h
index 118de64..d76547d 100644
--- a/quiche/quic/core/crypto/crypto_protocol.h
+++ b/quiche/quic/core/crypto/crypto_protocol.h
@@ -495,6 +495,8 @@
// Enable Failed Path Probe experiment
const QuicTag kFPPE = TAG('F', 'P', 'P', 'E');
+// Fix timeouts experiment.
+const QuicTag kFTOE = TAG('F', 'T', 'O', 'E');
// clang-format on
// These tags have a special form so that they appear either at the beginning
diff --git a/quiche/quic/core/http/end_to_end_test.cc b/quiche/quic/core/http/end_to_end_test.cc
index f753f56..978db9d 100644
--- a/quiche/quic/core/http/end_to_end_test.cc
+++ b/quiche/quic/core/http/end_to_end_test.cc
@@ -7863,6 +7863,26 @@
client_->Disconnect();
}
+TEST_P(EndToEndTest, FixTimeouts) {
+ client_extra_copts_.push_back(kFTOE);
+ ASSERT_TRUE(Initialize());
+ if (!version_.UsesTls()) {
+ return;
+ }
+ EXPECT_TRUE(client_->client()->WaitForHandshakeConfirmed());
+ // Verify handshake timeout has been removed on both endpoints.
+ QuicConnection* client_connection = GetClientConnection();
+ EXPECT_EQ(QuicConnectionPeer::GetIdleNetworkDetector(client_connection)
+ .handshake_timeout(),
+ QuicTime::Delta::Infinite());
+ server_thread_->Pause();
+ QuicConnection* server_connection = GetServerConnection();
+ EXPECT_EQ(QuicConnectionPeer::GetIdleNetworkDetector(server_connection)
+ .handshake_timeout(),
+ QuicTime::Delta::Infinite());
+ server_thread_->Resume();
+}
+
TEST_P(EndToEndTest, ClientMigrationAfterHalfwayServerMigration) {
use_preferred_address_ = true;
ASSERT_TRUE(Initialize());
diff --git a/quiche/quic/core/quic_connection.cc b/quiche/quic/core/quic_connection.cc
index e705044..bec9757 100644
--- a/quiche/quic/core/quic_connection.cc
+++ b/quiche/quic/core/quic_connection.cc
@@ -389,9 +389,21 @@
void QuicConnection::SetFromConfig(const QuicConfig& config) {
if (config.negotiated()) {
- // Handshake complete, set handshake timeout to Infinite.
- SetNetworkTimeouts(QuicTime::Delta::Infinite(),
- config.IdleNetworkTimeout());
+ if (ShouldFixTimeouts(config)) {
+ if (!IsHandshakeComplete()) {
+ QUIC_RELOADABLE_FLAG_COUNT_N(quic_fix_timeouts, 1, 2);
+ SetNetworkTimeouts(config.max_time_before_crypto_handshake(),
+ config.max_idle_time_before_crypto_handshake());
+ } else {
+ QUIC_BUG(set_from_config_after_handshake_complete)
+ << "SetFromConfig is called after Handshake complete";
+ // Network timeouts has been set by session on handshake complete.
+ }
+ } else {
+ // Handshake complete, set handshake timeout to Infinite.
+ SetNetworkTimeouts(QuicTime::Delta::Infinite(),
+ config.IdleNetworkTimeout());
+ }
idle_timeout_connection_close_behavior_ =
ConnectionCloseBehavior::SILENT_CLOSE;
if (perspective_ == Perspective::IS_SERVER) {
@@ -4657,6 +4669,9 @@
QUIC_BUG_IF(quic_bug_12714_29, idle_timeout > handshake_timeout)
<< "idle_timeout:" << idle_timeout.ToMilliseconds()
<< " handshake_timeout:" << handshake_timeout.ToMilliseconds();
+ QUIC_DVLOG(1) << ENDPOINT << "Setting network timeouts: "
+ << "handshake_timeout:" << handshake_timeout.ToMilliseconds()
+ << " idle_timeout:" << idle_timeout.ToMilliseconds();
// Adjust the idle timeout on client and server to prevent clients from
// sending requests to servers which have already closed the connection.
if (perspective_ == Perspective::IS_SERVER) {
@@ -6076,6 +6091,11 @@
perspective_ == Perspective::IS_SERVER && !default_path_.validated;
}
+bool QuicConnection::ShouldFixTimeouts(const QuicConfig& config) const {
+ return quic_fix_timeouts_ && version().UsesTls() &&
+ config.HasClientSentConnectionOption(kFTOE, perspective_);
+}
+
// TODO(danzh) Pass in path object or its reference of some sort to use this
// method to check anti-amplification limit on non-default path.
bool QuicConnection::LimitedByAmplificationFactor(QuicByteCount bytes) const {
diff --git a/quiche/quic/core/quic_connection.h b/quiche/quic/core/quic_connection.h
index 3a7e534..ca676b1 100644
--- a/quiche/quic/core/quic_connection.h
+++ b/quiche/quic/core/quic_connection.h
@@ -1412,6 +1412,8 @@
SerializeLargePacketNumberConnectionClosePacket(
QuicErrorCode error, const std::string& error_details);
+ bool ShouldFixTimeouts(const QuicConfig& config) const;
+
protected:
// Calls cancel() on all the alarms owned by this connection.
void CancelAllAlarms();
@@ -2512,6 +2514,8 @@
const bool quic_test_peer_addr_change_after_normalize_ =
GetQuicReloadableFlag(quic_test_peer_addr_change_after_normalize);
+
+ const bool quic_fix_timeouts_ = GetQuicReloadableFlag(quic_fix_timeouts);
};
} // namespace quic
diff --git a/quiche/quic/core/quic_connection_test.cc b/quiche/quic/core/quic_connection_test.cc
index 33b6002..b4f67c5 100644
--- a/quiche/quic/core/quic_connection_test.cc
+++ b/quiche/quic/core/quic_connection_test.cc
@@ -10318,6 +10318,75 @@
}
}
+TEST_P(QuicConnectionTest, FixTimeoutsClient) {
+ if (!connection_.version().UsesTls()) {
+ return;
+ }
+ set_perspective(Perspective::IS_CLIENT);
+ if (GetQuicReloadableFlag(quic_fix_timeouts)) {
+ EXPECT_CALL(visitor_, GetHandshakeState())
+ .WillRepeatedly(Return(HANDSHAKE_START));
+ }
+ QuicConfig config;
+ QuicTagVector connection_options;
+ connection_options.push_back(kFTOE);
+ config.SetConnectionOptionsToSend(connection_options);
+ QuicConfigPeer::SetNegotiated(&config, true);
+ QuicConfigPeer::SetReceivedOriginalConnectionId(&config,
+ connection_.connection_id());
+ QuicConfigPeer::SetReceivedInitialSourceConnectionId(
+ &config, connection_.connection_id());
+
+ EXPECT_CALL(*send_algorithm_, SetFromConfig(_, _)).Times(1);
+ connection_.SetFromConfig(config);
+ QuicIdleNetworkDetector& idle_network_detector =
+ QuicConnectionPeer::GetIdleNetworkDetector(&connection_);
+ if (GetQuicReloadableFlag(quic_fix_timeouts)) {
+ // Handshake timeout has not been removed yet.
+ EXPECT_NE(idle_network_detector.handshake_timeout(),
+ QuicTime::Delta::Infinite());
+ } else {
+ // Handshake timeout has been set to infinite.
+ EXPECT_EQ(idle_network_detector.handshake_timeout(),
+ QuicTime::Delta::Infinite());
+ }
+}
+
+TEST_P(QuicConnectionTest, FixTimeoutsServer) {
+ if (!connection_.version().UsesTls()) {
+ return;
+ }
+ set_perspective(Perspective::IS_SERVER);
+ if (GetQuicReloadableFlag(quic_fix_timeouts)) {
+ EXPECT_CALL(visitor_, GetHandshakeState())
+ .WillRepeatedly(Return(HANDSHAKE_START));
+ }
+ QuicConfig config;
+ quic::QuicTagVector initial_received_options;
+ initial_received_options.push_back(quic::kFTOE);
+ ASSERT_TRUE(
+ config.SetInitialReceivedConnectionOptions(initial_received_options));
+ QuicConfigPeer::SetNegotiated(&config, true);
+ QuicConfigPeer::SetReceivedOriginalConnectionId(&config,
+ connection_.connection_id());
+ QuicConfigPeer::SetReceivedInitialSourceConnectionId(&config,
+ QuicConnectionId());
+
+ EXPECT_CALL(*send_algorithm_, SetFromConfig(_, _)).Times(1);
+ connection_.SetFromConfig(config);
+ QuicIdleNetworkDetector& idle_network_detector =
+ QuicConnectionPeer::GetIdleNetworkDetector(&connection_);
+ if (GetQuicReloadableFlag(quic_fix_timeouts)) {
+ // Handshake timeout has not been removed yet.
+ EXPECT_NE(idle_network_detector.handshake_timeout(),
+ QuicTime::Delta::Infinite());
+ } else {
+ // Handshake timeout has been set to infinite.
+ EXPECT_EQ(idle_network_detector.handshake_timeout(),
+ QuicTime::Delta::Infinite());
+ }
+}
+
TEST_P(QuicConnectionTest, ClientParsesRetry) {
TestClientRetryHandling(/*invalid_retry_tag=*/false,
/*missing_original_id_in_config=*/false,
diff --git a/quiche/quic/core/quic_session.cc b/quiche/quic/core/quic_session.cc
index ae3e4f9..bba06b9 100644
--- a/quiche/quic/core/quic_session.cc
+++ b/quiche/quic/core/quic_session.cc
@@ -1796,6 +1796,12 @@
<< ENDPOINT << "Handshake completes without parameter negotiation.";
connection()->mutable_stats().handshake_completion_time =
connection()->clock()->ApproximateNow();
+ if (connection()->ShouldFixTimeouts(config_)) {
+ QUIC_RELOADABLE_FLAG_COUNT_N(quic_fix_timeouts, 2, 2);
+ // Handshake complete, set handshake timeout to Infinite.
+ connection()->SetNetworkTimeouts(QuicTime::Delta::Infinite(),
+ config_.IdleNetworkTimeout());
+ }
if (connection()->version().UsesTls() &&
perspective_ == Perspective::IS_SERVER) {
// Server sends HANDSHAKE_DONE to signal confirmation of the handshake
