[QUIC] Do not send multi-port probing when there's no active stream. Limit the max number of multi-port path creations to 5.
PiperOrigin-RevId: 492305120
diff --git a/quiche/quic/core/http/end_to_end_test.cc b/quiche/quic/core/http/end_to_end_test.cc
index d9bed33..b889402 100644
--- a/quiche/quic/core/http/end_to_end_test.cc
+++ b/quiche/quic/core/http/end_to_end_test.cc
@@ -5423,13 +5423,15 @@
}
TEST_P(EndToEndTest, ClientMultiPortConnection) {
- client_extra_copts_.push_back(kMPQC);
+ client_config_.SetClientConnectionOptions(QuicTagVector{kMPQC});
ASSERT_TRUE(Initialize());
if (!GetClientConnection()->connection_migration_use_new_cid()) {
return;
}
client_.reset(EndToEndTest::CreateQuicClient(nullptr));
QuicConnection* client_connection = GetClientConnection();
+ QuicSpdyClientStream* stream = client_->GetOrCreateStream();
+ ASSERT_TRUE(stream);
// Increase the probing frequency to speed up this test.
client_connection->SetMultiPortProbingInterval(
QuicTime::Delta::FromMilliseconds(100));
@@ -5463,6 +5465,7 @@
}));
// Verify that the previous path was retired.
EXPECT_EQ(1u, client_connection->GetStats().num_retire_connection_id_sent);
+ stream->Reset(QuicRstStreamErrorCode::QUIC_STREAM_NO_ERROR);
}
TEST_P(EndToEndPacketReorderingTest, ReorderedPathChallenge) {
diff --git a/quiche/quic/core/quic_connection.cc b/quiche/quic/core/quic_connection.cc
index 93cef1d..85f91a5 100644
--- a/quiche/quic/core/quic_connection.cc
+++ b/quiche/quic/core/quic_connection.cc
@@ -176,7 +176,7 @@
void OnAlarm() override {
QUICHE_DCHECK(connection_->connected());
QUIC_DLOG(INFO) << "Alternative path probing alarm fired";
- connection_->ProbeMultiPortPath();
+ connection_->MaybeProbeMultiPortPath();
}
};
@@ -673,10 +673,9 @@
UpdateReleaseTimeIntoFuture();
}
- multi_port_enabled_ =
+ if (perspective_ == Perspective::IS_CLIENT &&
connection_migration_use_new_cid_ &&
- config.HasClientSentConnectionOption(kMPQC, perspective_);
- if (multi_port_enabled_) {
+ config.HasClientRequestedIndependentOption(kMPQC, perspective_)) {
multi_port_stats_ = std::make_unique<MultiPortStats>();
}
}
@@ -1994,7 +1993,7 @@
if (!OnNewConnectionIdFrameInner(frame)) {
return false;
}
- if (perspective_ == Perspective::IS_CLIENT && multi_port_enabled_) {
+ if (multi_port_stats_ != nullptr) {
MaybeCreateMultiPortPath();
}
return true;
@@ -3929,14 +3928,21 @@
void QuicConnection::MaybeCreateMultiPortPath() {
QUICHE_DCHECK_EQ(Perspective::IS_CLIENT, perspective_);
+ QUIC_BUG_IF(quic_bug_12714_20, path_validator_.HasPendingPathValidation())
+ << "Pending validation exists when multi-port path is created.";
+ if (multi_port_stats_->num_multi_port_paths_created >=
+ kMaxNumMultiPortPaths) {
+ return;
+ }
auto path_context = visitor_->CreateContextForMultiPortPath();
- if (!path_context || path_validator_.HasPendingPathValidation()) {
+ if (!path_context) {
return;
}
auto multi_port_validation_result_delegate =
std::make_unique<MultiPortPathValidationResultDelegate>(this);
multi_port_probing_alarm_->Cancel();
multi_port_path_context_ = nullptr;
+ multi_port_stats_->num_multi_port_paths_created++;
ValidatePath(std::move(path_context),
std::move(multi_port_validation_result_delegate));
}
@@ -4495,6 +4501,7 @@
process_undecryptable_packets_alarm_->PermanentCancel();
discard_previous_one_rtt_keys_alarm_->PermanentCancel();
discard_zero_rtt_decryption_keys_alarm_->PermanentCancel();
+ multi_port_probing_alarm_->PermanentCancel();
blackhole_detector_.StopDetection(/*permanent=*/true);
idle_network_detector_.StopDetection();
}
@@ -6889,13 +6896,15 @@
}
}
-void QuicConnection::ProbeMultiPortPath() {
+void QuicConnection::MaybeProbeMultiPortPath() {
if (!connected_ || path_validator_.HasPendingPathValidation() ||
!multi_port_path_context_ ||
alternative_path_.self_address !=
multi_port_path_context_->self_address() ||
alternative_path_.peer_address !=
- multi_port_path_context_->peer_address()) {
+ multi_port_path_context_->peer_address() ||
+ !visitor_->ShouldKeepConnectionAlive() ||
+ multi_port_probing_alarm_->IsSet()) {
return;
}
auto multi_port_validation_result_delegate =
diff --git a/quiche/quic/core/quic_connection.h b/quiche/quic/core/quic_connection.h
index 63d4723..2efd08d 100644
--- a/quiche/quic/core/quic_connection.h
+++ b/quiche/quic/core/quic_connection.h
@@ -493,6 +493,8 @@
size_t num_multi_port_probe_failures_when_path_not_degrading = 0;
// number of multi-port probe failure when path is degrading
size_t num_multi_port_probe_failures_when_path_degrading = 0;
+ // number of total multi-port path creations in a connection
+ size_t num_multi_port_paths_created = 0;
};
// Sets connection parameters from the supplied |config|.
@@ -762,7 +764,7 @@
// Probe the existing alternative path. Does not create a new alternative
// path. This method is the callback for |multi_port_probing_alarm_|.
- void ProbeMultiPortPath();
+ virtual void MaybeProbeMultiPortPath();
// Accessors
void set_visitor(QuicConnectionVisitorInterface* visitor) {
@@ -805,7 +807,6 @@
QuicByteCount max_packet_length() const;
void SetMaxPacketLength(QuicByteCount length);
- bool multi_port_enabled() const { return multi_port_enabled_; }
size_t mtu_probe_count() const { return mtu_probe_count_; }
bool connected() const { return connected_; }
@@ -2253,8 +2254,6 @@
std::unique_ptr<QuicPathValidationContext> multi_port_path_context_;
- bool multi_port_enabled_ = false;
-
QuicTime::Delta multi_port_probing_interval_;
std::unique_ptr<MultiPortStats> multi_port_stats_;
diff --git a/quiche/quic/core/quic_connection_test.cc b/quiche/quic/core/quic_connection_test.cc
index fdb0a0d..100dd22 100644
--- a/quiche/quic/core/quic_connection_test.cc
+++ b/quiche/quic/core/quic_connection_test.cc
@@ -13008,10 +13008,11 @@
&connection_, kNewSelfAddress, connection_.peer_address()));
}
-TEST_P(QuicConnectionTest, MultiPortCreation) {
+TEST_P(QuicConnectionTest, MultiPortConnection) {
set_perspective(Perspective::IS_CLIENT);
QuicConfig config;
- config.SetConnectionOptionsToSend(QuicTagVector{kMPQC, kRVCM});
+ config.SetConnectionOptionsToSend(QuicTagVector{kRVCM});
+ config.SetClientConnectionOptions(QuicTagVector{kMPQC});
EXPECT_CALL(*send_algorithm_, SetFromConfig(_, _));
connection_.SetFromConfig(config);
if (!connection_.connection_migration_use_new_cid()) {
@@ -13030,6 +13031,7 @@
EXPECT_NE(kNewSelfAddress, self_address);
TestPacketWriter new_writer(version(), &clock_, Perspective::IS_CLIENT);
+ EXPECT_CALL(visitor_, ShouldKeepConnectionAlive()).WillOnce(Return(false));
QuicNewConnectionIdFrame frame;
frame.connection_id = TestConnectionId(1234);
ASSERT_NE(frame.connection_id, connection_.connection_id());
@@ -13055,7 +13057,7 @@
QuicFrames frames;
frames.push_back(QuicFrame(QuicPathResponseFrame(
- 99, new_writer.path_challenge_frames().front().data_buffer)));
+ 99, new_writer.path_challenge_frames().back().data_buffer)));
ProcessFramesPacketWithAddresses(frames, kNewSelfAddress, kPeerAddress,
ENCRYPTION_FORWARD_SECURE);
// No migration should happen and the alternative path should still be alive.
@@ -13071,6 +13073,46 @@
EXPECT_EQ(kTestRTT,
stats->rtt_stats_when_default_path_degrading.latest_rtt());
+ // When there's no active request, the probing shouldn't happen. But the
+ // probing context should be saved.
+ EXPECT_CALL(visitor_, ShouldKeepConnectionAlive()).WillOnce(Return(false));
+ connection_.GetMultiPortProbingAlarm()->Fire();
+ EXPECT_FALSE(connection_.HasPendingPathValidation());
+ EXPECT_FALSE(connection_.GetMultiPortProbingAlarm()->IsSet());
+
+ // Simulate the situation where a new request stream is created.
+ EXPECT_CALL(visitor_, ShouldKeepConnectionAlive())
+ .WillRepeatedly(Return(true));
+ random_generator_.ChangeValue();
+ connection_.MaybeProbeMultiPortPath();
+ EXPECT_TRUE(QuicConnectionPeer::IsAlternativePath(
+ &connection_, kNewSelfAddress, connection_.peer_address()));
+ EXPECT_TRUE(alt_path->validated);
+ // Fake a response delay.
+ clock_.AdvanceTime(kTestRTT);
+ QuicFrames frames2;
+ frames2.push_back(QuicFrame(QuicPathResponseFrame(
+ 99, new_writer.path_challenge_frames().back().data_buffer)));
+ ProcessFramesPacketWithAddresses(frames2, kNewSelfAddress, kPeerAddress,
+ ENCRYPTION_FORWARD_SECURE);
+ // No migration should happen and the alternative path should still be alive.
+ EXPECT_FALSE(connection_.HasPendingPathValidation());
+ EXPECT_TRUE(QuicConnectionPeer::IsAlternativePath(
+ &connection_, kNewSelfAddress, connection_.peer_address()));
+ EXPECT_TRUE(alt_path->validated);
+ EXPECT_EQ(1, stats->num_path_degrading);
+ EXPECT_EQ(0, stats->num_multi_port_probe_failures_when_path_degrading);
+ EXPECT_EQ(kTestRTT, stats->rtt_stats.latest_rtt());
+ EXPECT_EQ(kTestRTT,
+ stats->rtt_stats_when_default_path_degrading.latest_rtt());
+
+ EXPECT_TRUE(connection_.GetMultiPortProbingAlarm()->IsSet());
+ // Since there's already a scheduled probing alarm, manual calls won't have
+ // any effect.
+ connection_.MaybeProbeMultiPortPath();
+ EXPECT_FALSE(connection_.HasPendingPathValidation());
+
+ // Simulate the case where the path validation fails after retries.
connection_.GetMultiPortProbingAlarm()->Fire();
EXPECT_TRUE(connection_.HasPendingPathValidation());
EXPECT_TRUE(QuicConnectionPeer::IsAlternativePath(
@@ -13091,6 +13133,116 @@
EXPECT_EQ(0, stats->num_multi_port_probe_failures_when_path_not_degrading);
}
+TEST_P(QuicConnectionTest, TooManyMultiPortPathCreations) {
+ set_perspective(Perspective::IS_CLIENT);
+ QuicConfig config;
+ config.SetConnectionOptionsToSend(QuicTagVector{kRVCM});
+ config.SetClientConnectionOptions(QuicTagVector{kMPQC});
+ EXPECT_CALL(*send_algorithm_, SetFromConfig(_, _));
+ connection_.SetFromConfig(config);
+ if (!connection_.connection_migration_use_new_cid()) {
+ return;
+ }
+ connection_.CreateConnectionIdManager();
+ connection_.SetDefaultEncryptionLevel(ENCRYPTION_FORWARD_SECURE);
+ connection_.OnHandshakeComplete();
+
+ EXPECT_CALL(visitor_, OnPathDegrading());
+ connection_.OnPathDegradingDetected();
+
+ auto self_address = connection_.self_address();
+ const QuicSocketAddress kNewSelfAddress(self_address.host(),
+ self_address.port() + 1);
+ EXPECT_NE(kNewSelfAddress, self_address);
+ TestPacketWriter new_writer(version(), &clock_, Perspective::IS_CLIENT);
+
+ EXPECT_CALL(visitor_, ShouldKeepConnectionAlive())
+ .WillRepeatedly(Return(true));
+
+ QuicNewConnectionIdFrame frame;
+ frame.connection_id = TestConnectionId(1234);
+ ASSERT_NE(frame.connection_id, connection_.connection_id());
+ frame.stateless_reset_token =
+ QuicUtils::GenerateStatelessResetToken(frame.connection_id);
+ frame.retire_prior_to = 0u;
+ frame.sequence_number = 1u;
+ EXPECT_CALL(visitor_, CreateContextForMultiPortPath())
+ .WillRepeatedly(Return(
+ testing::ByMove(std::make_unique<TestQuicPathValidationContext>(
+ kNewSelfAddress, connection_.peer_address(), &new_writer))));
+ EXPECT_TRUE(connection_.OnNewConnectionIdFrame(frame));
+ EXPECT_TRUE(connection_.HasPendingPathValidation());
+ EXPECT_TRUE(QuicConnectionPeer::IsAlternativePath(
+ &connection_, kNewSelfAddress, connection_.peer_address()));
+ auto* alt_path = QuicConnectionPeer::GetAlternativePath(&connection_);
+ EXPECT_FALSE(alt_path->validated);
+
+ EXPECT_TRUE(connection_.HasPendingPathValidation());
+ EXPECT_TRUE(QuicConnectionPeer::IsAlternativePath(
+ &connection_, kNewSelfAddress, connection_.peer_address()));
+ for (size_t i = 0; i < QuicPathValidator::kMaxRetryTimes + 1; ++i) {
+ clock_.AdvanceTime(QuicTime::Delta::FromMilliseconds(3 * kInitialRttMs));
+ static_cast<TestAlarmFactory::TestAlarm*>(
+ QuicPathValidatorPeer::retry_timer(
+ QuicConnectionPeer::path_validator(&connection_)))
+ ->Fire();
+ }
+
+ auto stats = connection_.multi_port_stats();
+ EXPECT_FALSE(connection_.HasPendingPathValidation());
+ EXPECT_FALSE(QuicConnectionPeer::IsAlternativePath(
+ &connection_, kNewSelfAddress, connection_.peer_address()));
+ EXPECT_EQ(1, stats->num_path_degrading);
+ EXPECT_EQ(1, stats->num_multi_port_probe_failures_when_path_degrading);
+
+ uint64_t connection_id = 1235;
+ for (size_t i = 0; i < kMaxNumMultiPortPaths - 1; ++i) {
+ QuicNewConnectionIdFrame frame;
+ frame.connection_id = TestConnectionId(connection_id + i);
+ ASSERT_NE(frame.connection_id, connection_.connection_id());
+ frame.stateless_reset_token =
+ QuicUtils::GenerateStatelessResetToken(frame.connection_id);
+ frame.retire_prior_to = 0u;
+ frame.sequence_number = i + 2;
+ EXPECT_CALL(visitor_, CreateContextForMultiPortPath())
+ .WillRepeatedly(Return(
+ testing::ByMove(std::make_unique<TestQuicPathValidationContext>(
+ kNewSelfAddress, connection_.peer_address(), &new_writer))));
+ EXPECT_TRUE(connection_.OnNewConnectionIdFrame(frame));
+ EXPECT_TRUE(connection_.HasPendingPathValidation());
+ EXPECT_TRUE(QuicConnectionPeer::IsAlternativePath(
+ &connection_, kNewSelfAddress, connection_.peer_address()));
+ EXPECT_FALSE(alt_path->validated);
+
+ for (size_t j = 0; j < QuicPathValidator::kMaxRetryTimes + 1; ++j) {
+ clock_.AdvanceTime(QuicTime::Delta::FromMilliseconds(3 * kInitialRttMs));
+ static_cast<TestAlarmFactory::TestAlarm*>(
+ QuicPathValidatorPeer::retry_timer(
+ QuicConnectionPeer::path_validator(&connection_)))
+ ->Fire();
+ }
+
+ EXPECT_FALSE(connection_.HasPendingPathValidation());
+ EXPECT_FALSE(QuicConnectionPeer::IsAlternativePath(
+ &connection_, kNewSelfAddress, connection_.peer_address()));
+ EXPECT_EQ(1, stats->num_path_degrading);
+ EXPECT_EQ(i + 2, stats->num_multi_port_probe_failures_when_path_degrading);
+ }
+
+ // The 6th attemp should fail.
+ QuicNewConnectionIdFrame frame2;
+ frame2.connection_id = TestConnectionId(1239);
+ ASSERT_NE(frame2.connection_id, connection_.connection_id());
+ frame2.stateless_reset_token =
+ QuicUtils::GenerateStatelessResetToken(frame2.connection_id);
+ frame2.retire_prior_to = 0u;
+ frame2.sequence_number = 6u;
+ EXPECT_TRUE(connection_.OnNewConnectionIdFrame(frame2));
+ EXPECT_FALSE(connection_.HasPendingPathValidation());
+ EXPECT_EQ(kMaxNumMultiPortPaths,
+ stats->num_multi_port_probe_failures_when_path_degrading);
+}
+
TEST_P(QuicConnectionTest, SingleAckInPacket) {
EXPECT_CALL(visitor_, OnSuccessfulVersionNegotiation(_));
EXPECT_CALL(visitor_, OnConnectionClosed(_, _));
diff --git a/quiche/quic/core/quic_constants.h b/quiche/quic/core/quic_constants.h
index 13fa288..4589189 100644
--- a/quiche/quic/core/quic_constants.h
+++ b/quiche/quic/core/quic_constants.h
@@ -324,6 +324,8 @@
inline constexpr QuicTime::Delta kDefaultMultiPortProbingInterval =
QuicTime::Delta::FromSeconds(3);
+inline constexpr size_t kMaxNumMultiPortPaths = 5;
+
} // namespace quic
#endif // QUICHE_QUIC_CORE_QUIC_CONSTANTS_H_
diff --git a/quiche/quic/core/quic_session.cc b/quiche/quic/core/quic_session.cc
index 242b9fc..158db55 100644
--- a/quiche/quic/core/quic_session.cc
+++ b/quiche/quic/core/quic_session.cc
@@ -1816,6 +1816,7 @@
}
void QuicSession::ActivateStream(std::unique_ptr<QuicStream> stream) {
+ const bool should_keep_alive = ShouldKeepConnectionAlive();
QuicStreamId stream_id = stream->id();
bool is_static = stream->is_static();
QUIC_DVLOG(1) << ENDPOINT << "num_streams: " << stream_map_.size()
@@ -1831,6 +1832,11 @@
stream_id_manager_.ActivateStream(
/*is_incoming=*/IsIncomingStream(stream_id));
}
+ if (perspective() == Perspective::IS_CLIENT &&
+ connection()->multi_port_stats() != nullptr && !should_keep_alive &&
+ ShouldKeepConnectionAlive()) {
+ connection()->MaybeProbeMultiPortPath();
+ }
}
QuicStreamId QuicSession::GetNextOutgoingBidirectionalStreamId() {
diff --git a/quiche/quic/core/quic_session_test.cc b/quiche/quic/core/quic_session_test.cc
index a55035f..d663cbd 100644
--- a/quiche/quic/core/quic_session_test.cc
+++ b/quiche/quic/core/quic_session_test.cc
@@ -2086,6 +2086,23 @@
&session_, GetNthClientInitiatedBidirectionalId(1)));
}
+TEST_P(QuicSessionTestClient, NewStreamCreationResumesMultiPortProbing) {
+ session_.config()->SetConnectionOptionsToSend({kRVCM});
+ session_.config()->SetClientConnectionOptions({kMPQC});
+ session_.Initialize();
+ connection_->CreateConnectionIdManager();
+ connection_->SetDefaultEncryptionLevel(ENCRYPTION_FORWARD_SECURE);
+ connection_->OnHandshakeComplete();
+ session_.OnConfigNegotiated();
+
+ if (!connection_->connection_migration_use_new_cid()) {
+ return;
+ }
+
+ EXPECT_CALL(*connection_, MaybeProbeMultiPortPath());
+ session_.CreateOutgoingBidirectionalStream();
+}
+
TEST_P(QuicSessionTestClient, InvalidSessionFlowControlWindowInHandshake) {
// Test that receipt of an invalid (< default for gQUIC, < current for TLS)
// session flow control window from the peer results in the connection being
diff --git a/quiche/quic/core/quic_stream_test.cc b/quiche/quic/core/quic_stream_test.cc
index 1e77c5a..a0837be 100644
--- a/quiche/quic/core/quic_stream_test.cc
+++ b/quiche/quic/core/quic_stream_test.cc
@@ -113,6 +113,8 @@
stream_ = new StrictMock<TestStream>(kTestStreamId, session_.get(),
BIDIRECTIONAL);
EXPECT_NE(nullptr, stream_);
+ EXPECT_CALL(*session_, ShouldKeepConnectionAlive())
+ .WillRepeatedly(Return(true));
// session_ now owns stream_.
session_->ActivateStream(absl::WrapUnique(stream_));
// Ignore resetting when session_ is terminated.
diff --git a/quiche/quic/test_tools/quic_test_utils.h b/quiche/quic/test_tools/quic_test_utils.h
index 4c8c9b0..b2d8f08 100644
--- a/quiche/quic/test_tools/quic_test_utils.h
+++ b/quiche/quic/test_tools/quic_test_utils.h
@@ -636,6 +636,7 @@
MOCK_METHOD(bool, SendConnectivityProbingPacket,
(QuicPacketWriter*, const QuicSocketAddress& peer_address),
(override));
+ MOCK_METHOD(void, MaybeProbeMultiPortPath, (), (override));
MOCK_METHOD(void, OnSendConnectionState, (const CachedNetworkParameters&),
(override));
diff --git a/quiche/quic/tools/quic_simple_client_session.cc b/quiche/quic/tools/quic_simple_client_session.cc
index 013167c..2350052 100644
--- a/quiche/quic/tools/quic_simple_client_session.cc
+++ b/quiche/quic/tools/quic_simple_client_session.cc
@@ -38,7 +38,7 @@
std::unique_ptr<QuicPathValidationContext>
QuicSimpleClientSession::CreateContextForMultiPortPath() {
- if (!network_helper_ || !connection()->multi_port_enabled()) {
+ if (!network_helper_ || connection()->multi_port_stats() == nullptr) {
return nullptr;
}
auto self_address = connection()->self_address();
