From 232c19ed916122dbb792d6a6319867b847e21b13 Mon Sep 17 00:00:00 2001 From: Fredy Wijaya Date: Fri, 8 Nov 2024 12:11:54 -0600 Subject: [PATCH] quic: Don't delay TCP attempt when HTTP/3 status is unknown (#37040) Commit Message: When HTTP/3 is not known to work, we should attempt both QUIC and TCP at the same time instead of giving QUIC a head start and delaying the TCP connection attempt. Additional Description: In Envoy Mobile, the HTTP/3 status will be reset upon network change. Risk Level: low (runtime guarded and the default is false) Testing: unit and integration tests Docs Changes: n/a Release Notes: n/a Platform Specific Features: quic, mobile Optional Runtime guard: `envoy.reloadable_features.quic_no_tcp_delay` (default is false) --------- Signed-off-by: Fredy Wijaya --- envoy/http/http_server_properties_cache.h | 9 + .../examples/cc/fetch_client/fetch_client.cc | 1 + mobile/library/common/internal_engine.cc | 8 + mobile/test/cc/unit/fetch_client_test.cc | 10 +- .../integration/client_integration_test.cc | 4 +- source/common/http/conn_pool_grid.cc | 14 +- .../common/http/http3_status_tracker_impl.cc | 4 + .../common/http/http3_status_tracker_impl.h | 4 + .../http/http_server_properties_cache_impl.cc | 8 + .../http/http_server_properties_cache_impl.h | 1 + source/common/runtime/runtime_features.cc | 2 + test/common/http/conn_pool_grid_test.cc | 285 ++++++++++++++++++ .../http/http3_status_tracker_impl_test.cc | 17 ++ .../mocks/http/http_server_properties_cache.h | 1 + 14 files changed, 361 insertions(+), 7 deletions(-) diff --git a/envoy/http/http_server_properties_cache.h b/envoy/http/http_server_properties_cache.h index 24edbfae378b..8690a5773c06 100644 --- a/envoy/http/http_server_properties_cache.h +++ b/envoy/http/http_server_properties_cache.h @@ -97,12 +97,16 @@ class HttpServerPropertiesCache { public: virtual ~Http3StatusTracker() = default; + // Returns true if HTTP/3 status is pending. + virtual bool isHttp3Pending() const PURE; // Returns true if HTTP/3 is broken. virtual bool isHttp3Broken() const PURE; // Returns true if HTTP/3 is confirmed to be working. virtual bool isHttp3Confirmed() const PURE; // Returns true if HTTP/3 has failed recently. virtual bool hasHttp3FailedRecently() const PURE; + // Marks HTTP/3 status as pending. + virtual void markHttp3Pending() PURE; // Marks HTTP/3 broken for a period of time, subject to backoff. virtual void markHttp3Broken() PURE; // Marks HTTP/3 as confirmed to be working and resets the backoff timeout. @@ -179,6 +183,11 @@ class HttpServerPropertiesCache { * Changes any origins with status "Broken" for HTTP/3 to "Failed Recently" */ virtual void resetBrokenness() PURE; + + /** + * Changes any origin status for HTTP/3 to "Pending". + */ + virtual void resetStatus() PURE; }; using HttpServerPropertiesCacheSharedPtr = std::shared_ptr; diff --git a/mobile/examples/cc/fetch_client/fetch_client.cc b/mobile/examples/cc/fetch_client/fetch_client.cc index 6bba7ebfcaee..bebc9b9564b5 100644 --- a/mobile/examples/cc/fetch_client/fetch_client.cc +++ b/mobile/examples/cc/fetch_client/fetch_client.cc @@ -122,6 +122,7 @@ void Fetch::runEngine(absl::Notification& engine_running, Platform::EngineBuilder engine_builder; engine_builder.setLogLevel(Logger::Logger::trace); engine_builder.addRuntimeGuard("dns_cache_set_ip_version_to_remove", true); + engine_builder.addRuntimeGuard("quic_no_tcp_delay", true); engine_builder.setOnEngineRunning([&engine_running]() { engine_running.Notify(); }); if (!quic_hints.empty()) { engine_builder.enableHttp3(true); diff --git a/mobile/library/common/internal_engine.cc b/mobile/library/common/internal_engine.cc index 0f99af112a68..35d8e296976f 100644 --- a/mobile/library/common/internal_engine.cc +++ b/mobile/library/common/internal_engine.cc @@ -303,6 +303,14 @@ void InternalEngine::onDefaultNetworkChanged(NetworkType network) { [](Http::HttpServerPropertiesCache& cache) { cache.resetBrokenness(); }; cache_manager.forEachThreadLocalCache(clear_brokenness); } + if (Runtime::runtimeFeatureEnabled("envoy.reloadable_features.quic_no_tcp_delay")) { + Http::HttpServerPropertiesCacheManager& cache_manager = + server_->httpServerPropertiesCacheManager(); + + Http::HttpServerPropertiesCacheManager::CacheFn reset_status = + [](Http::HttpServerPropertiesCache& cache) { cache.resetStatus(); }; + cache_manager.forEachThreadLocalCache(reset_status); + } connectivity_manager_->refreshDns(configuration, true); }); } diff --git a/mobile/test/cc/unit/fetch_client_test.cc b/mobile/test/cc/unit/fetch_client_test.cc index df3cde261ee5..4df15ed05f28 100644 --- a/mobile/test/cc/unit/fetch_client_test.cc +++ b/mobile/test/cc/unit/fetch_client_test.cc @@ -18,10 +18,14 @@ TEST(FetchClientTest, Http2) { TEST(FetchClientTest, Http3) { Envoy::Fetch client; std::vector protocols; - ASSERT_EQ(client.fetch({"https://www.google.com/"}, {"www.google.com"}, protocols), + ASSERT_EQ(client.fetch({"https://www.google.com/", "https://www.google.com/"}, {"www.google.com"}, + protocols), ENVOY_SUCCESS); - // TODO(fredyw): In CI, HTTP/3 does not work and will use HTTP/2 instead. - ASSERT_GT(protocols.front(), Http::Protocol::Http11); + // The first request could either be HTTP/2 or HTTP/3 because we no longer give HTTP/3 a head + // start. + ASSERT_GE(protocols.at(0), Http::Protocol::Http2); + // TODO(fredyw): In EngFlow CI, HTTP/3 does not work and will use HTTP/2 instead. + ASSERT_GE(protocols.at(1), Http::Protocol::Http2); } } // namespace diff --git a/mobile/test/common/integration/client_integration_test.cc b/mobile/test/common/integration/client_integration_test.cc index 756535981459..e06ba9882633 100644 --- a/mobile/test/common/integration/client_integration_test.cc +++ b/mobile/test/common/integration/client_integration_test.cc @@ -77,6 +77,9 @@ class ClientIntegrationTest } void initialize() override { + builder_.addRuntimeGuard("dns_cache_set_ip_version_to_remove", true); + builder_.addRuntimeGuard("quic_no_tcp_delay", true); + if (getCodecType() == Http::CodecType::HTTP3) { setUpstreamProtocol(Http::CodecType::HTTP3); builder_.enablePlatformCertificatesValidation(true); @@ -106,7 +109,6 @@ class ClientIntegrationTest } else if (getCodecType() == Http::CodecType::HTTP2) { default_request_headers_.setScheme("https"); } - builder_.addRuntimeGuard("dns_cache_set_ip_version_to_remove", true); } void SetUp() override { diff --git a/source/common/http/conn_pool_grid.cc b/source/common/http/conn_pool_grid.cc index cc59cd0ebc51..16cb5b71c334 100644 --- a/source/common/http/conn_pool_grid.cc +++ b/source/common/http/conn_pool_grid.cc @@ -417,9 +417,17 @@ ConnectionPool::Cancellable* ConnectivityGrid::newStream(Http::ResponseDecoder& bool delay_tcp_attempt = true; bool delay_alternate_http3_attempt = true; if (shouldAttemptHttp3() && options.can_use_http3_) { - if (getHttp3StatusTracker().hasHttp3FailedRecently()) { - overriding_options.can_send_early_data_ = false; - delay_tcp_attempt = false; + if (Runtime::runtimeFeatureEnabled("envoy.reloadable_features.quic_no_tcp_delay")) { + if (getHttp3StatusTracker().isHttp3Pending() || + getHttp3StatusTracker().hasHttp3FailedRecently()) { + overriding_options.can_send_early_data_ = false; + delay_tcp_attempt = false; + } + } else { + if (getHttp3StatusTracker().hasHttp3FailedRecently()) { + overriding_options.can_send_early_data_ = false; + delay_tcp_attempt = false; + } } if (http3_pool_ && http3_alternate_pool_ && !http3_pool_->hasActiveConnections() && http3_alternate_pool_->hasActiveConnections()) { diff --git a/source/common/http/http3_status_tracker_impl.cc b/source/common/http/http3_status_tracker_impl.cc index 4f0dc6db28b9..e6297e9db2ec 100644 --- a/source/common/http/http3_status_tracker_impl.cc +++ b/source/common/http/http3_status_tracker_impl.cc @@ -14,6 +14,8 @@ const int MaxConsecutiveBrokenCount = 17; Http3StatusTrackerImpl::Http3StatusTrackerImpl(Event::Dispatcher& dispatcher) : expiration_timer_(dispatcher.createTimer([this]() -> void { onExpirationTimeout(); })) {} +bool Http3StatusTrackerImpl::isHttp3Pending() const { return state_ == State::Pending; } + bool Http3StatusTrackerImpl::isHttp3Broken() const { return state_ == State::Broken; } bool Http3StatusTrackerImpl::isHttp3Confirmed() const { return state_ == State::Confirmed; } @@ -22,6 +24,8 @@ bool Http3StatusTrackerImpl::hasHttp3FailedRecently() const { return state_ == State::FailedRecently; } +void Http3StatusTrackerImpl::markHttp3Pending() { state_ = State::Pending; } + void Http3StatusTrackerImpl::markHttp3Broken() { state_ = State::Broken; if (!expiration_timer_->enabled()) { diff --git a/source/common/http/http3_status_tracker_impl.h b/source/common/http/http3_status_tracker_impl.h index b27ad3d69f35..3ea8200f0e17 100644 --- a/source/common/http/http3_status_tracker_impl.h +++ b/source/common/http/http3_status_tracker_impl.h @@ -15,12 +15,16 @@ class Http3StatusTrackerImpl : public HttpServerPropertiesCache::Http3StatusTrac public: explicit Http3StatusTrackerImpl(Event::Dispatcher& dispatcher); + // Returns true if HTTP/3 status is pending. + bool isHttp3Pending() const override; // Returns true if HTTP/3 is broken. bool isHttp3Broken() const override; // Returns true if HTTP/3 is confirmed to be working. bool isHttp3Confirmed() const override; // Returns true if HTTP/3 has failed recently. bool hasHttp3FailedRecently() const override; + // Marks HTTP/3 status as pending. + void markHttp3Pending() override; // Marks HTTP/3 broken for a period of time, subject to backoff. void markHttp3Broken() override; // Marks HTTP/3 as confirmed to be working and resets the backoff timeout. diff --git a/source/common/http/http_server_properties_cache_impl.cc b/source/common/http/http_server_properties_cache_impl.cc index 1b6cc15ba6e1..b3f6f3103c5e 100644 --- a/source/common/http/http_server_properties_cache_impl.cc +++ b/source/common/http/http_server_properties_cache_impl.cc @@ -304,6 +304,14 @@ void HttpServerPropertiesCacheImpl::resetBrokenness() { } } +void HttpServerPropertiesCacheImpl::resetStatus() { + for (const std::pair& protocol : protocols_) { + if (protocol.second.h3_status_tracker) { + protocol.second.h3_status_tracker->markHttp3Pending(); + } + } +} + absl::string_view HttpServerPropertiesCacheImpl::getCanonicalSuffix(absl::string_view hostname) { for (const std::string& suffix : canonical_suffixes_) { if (absl::EndsWith(hostname, suffix)) { diff --git a/source/common/http/http_server_properties_cache_impl.h b/source/common/http/http_server_properties_cache_impl.h index d728f21b8267..7bdd8c78df25 100644 --- a/source/common/http/http_server_properties_cache_impl.h +++ b/source/common/http/http_server_properties_cache_impl.h @@ -92,6 +92,7 @@ class HttpServerPropertiesCacheImpl : public HttpServerPropertiesCache, HttpServerPropertiesCache::Http3StatusTracker& getOrCreateHttp3StatusTracker(const Origin& origin) override; void resetBrokenness() override; + void resetStatus() override; private: // Time source used to check expiration of entries. diff --git a/source/common/runtime/runtime_features.cc b/source/common/runtime/runtime_features.cc index 16491995346b..8997273ae5d1 100644 --- a/source/common/runtime/runtime_features.cc +++ b/source/common/runtime/runtime_features.cc @@ -151,6 +151,8 @@ FALSE_RUNTIME_GUARD(envoy_restart_features_xds_failover_support); FALSE_RUNTIME_GUARD(envoy_reloadable_features_dns_cache_set_ip_version_to_remove); // TODO(alyssawilk): evaluate and make this a config knob or remove. FALSE_RUNTIME_GUARD(envoy_reloadable_features_reset_brokenness_on_nework_change); +// TODO(fredyw): evaluate and either make this a config knob or remove. +FALSE_RUNTIME_GUARD(envoy_reloadable_features_quic_no_tcp_delay); // Adding runtime flag to use balsa_parser for http_inspector. FALSE_RUNTIME_GUARD(envoy_reloadable_features_http_inspector_use_balsa_parser); diff --git a/test/common/http/conn_pool_grid_test.cc b/test/common/http/conn_pool_grid_test.cc index 7af825a4eb60..4a794b5143ac 100644 --- a/test/common/http/conn_pool_grid_test.cc +++ b/test/common/http/conn_pool_grid_test.cc @@ -18,6 +18,7 @@ #include "test/mocks/ssl/mocks.h" #include "test/mocks/upstream/cluster_info.h" #include "test/test_common/simulated_time_system.h" +#include "test/test_common/test_runtime.h" #include "test/test_common/threadsafe_singleton_injector.h" #include "test/test_common/utility.h" @@ -132,6 +133,10 @@ class ConnectivityGridForTest : public ConnectivityGrid { ConnectionPool::Callbacks* callbacks(int index = 0) { return callbacks_[index]; } + size_t callbackSize() { return callbacks_.size(); } + + void removeCallbacks() { callbacks_.clear(); } + bool isHttp3Confirmed() const { ASSERT(host_->address()->type() == Network::Address::Type::Ip); HttpServerPropertiesCache::Origin origin{"https", host_->hostname(), @@ -374,6 +379,286 @@ TEST_F(ConnectivityGridTest, ThreeParallelConnections) { /*can_use_http3_=*/true}); } +TEST_F(ConnectivityGridTest, ParallelConnectionsNoTcpDelayQuicSucceedsFirstThenQuic) { + TestScopedRuntime scoped_runtime; + scoped_runtime.mergeValues({{"envoy.reloadable_features.quic_no_tcp_delay", "true"}}); + + initialize(); + grid_->alternate_immediate_ = false; + addHttp3AlternateProtocol(); + EXPECT_EQ(grid_->http3Pool(), nullptr); + + EXPECT_LOG_CONTAINS_ALL_OF( + Envoy::ExpectedLogMessages( + {{"trace", "http3 pool attempting to create a new stream to host 'hostname'"}, + {"trace", "http2 pool attempting to create a new stream to host 'hostname'"}}), + { + grid_->newStream(decoder_, callbacks_, + {/*can_send_early_data=*/false, + /*can_use_http3_=*/true}); + }); + + EXPECT_NE(grid_->http3Pool(), nullptr); + EXPECT_NE(grid_->http2Pool(), nullptr); + // Contains QUIC and TCP. + ASSERT_EQ(grid_->callbackSize(), 2); + + // QUIC finishes first. + grid_->onHandshakeComplete(); + EXPECT_TRUE(grid_->isHttp3Confirmed()); + EXPECT_CALL(callbacks_.pool_ready_, ready()); + EXPECT_LOG_CONTAINS("trace", "http3 pool successfully connected to host 'hostname'", + grid_->callbacks(0)->onPoolReady(encoder_, host_, info_, absl::nullopt)); + // The in-flight TCP connection attempt after HTTP/3 pool successfully connected to the host is + // cancelled. + + grid_->removeCallbacks(); + + // Create a new stream now that we have an updated HTTP/3 status. + EXPECT_LOG_CONTAINS("trace", "http3 pool attempting to create a new stream to host 'hostname'", + grid_->newStream(decoder_, callbacks_, + {/*can_send_early_data=*/false, + /*can_use_http3_=*/true})); + + // Because HTTP/3 is confirmed, we skip attempting a TCP connection, so there is only one callback + // for QUIC. + ASSERT_EQ(grid_->callbackSize(), 1); + grid_->onHandshakeComplete(); + EXPECT_TRUE(grid_->isHttp3Confirmed()); + EXPECT_CALL(callbacks_.pool_ready_, ready()); + EXPECT_LOG_CONTAINS("trace", "http3 pool successfully connected to host 'hostname'", + grid_->callbacks(0)->onPoolReady(encoder_, host_, info_, absl::nullopt)); +} + +TEST_F(ConnectivityGridTest, ParallelConnectionsNoTcpDelayTcpSucceedsFirstThenQuic) { + TestScopedRuntime scoped_runtime; + scoped_runtime.mergeValues({{"envoy.reloadable_features.quic_no_tcp_delay", "true"}}); + + initialize(); + grid_->alternate_immediate_ = false; + addHttp3AlternateProtocol(); + EXPECT_EQ(grid_->http3Pool(), nullptr); + + EXPECT_LOG_CONTAINS_ALL_OF( + Envoy::ExpectedLogMessages( + {{"trace", "http3 pool attempting to create a new stream to host 'hostname'"}, + {"trace", "http2 pool attempting to create a new stream to host 'hostname'"}}), + { + grid_->newStream(decoder_, callbacks_, + {/*can_send_early_data=*/false, + /*can_use_http3_=*/true}); + }); + + EXPECT_NE(grid_->http3Pool(), nullptr); + EXPECT_NE(grid_->http2Pool(), nullptr); + // Contains QUIC and TCP. + ASSERT_EQ(grid_->callbackSize(), 2); + + // TCP finishes first. + EXPECT_CALL(callbacks_.pool_ready_, ready()); + EXPECT_LOG_CONTAINS("trace", "http2 pool successfully connected to host 'hostname'", + grid_->callbacks(1)->onPoolReady(encoder_, host_, info_, absl::nullopt)); + // HTTP/3 is still not confirmed and also not marked broken. + EXPECT_FALSE(grid_->isHttp3Confirmed()); + EXPECT_FALSE(grid_->isHttp3Broken()); + + // QUIC finishes second. + grid_->onHandshakeComplete(); + EXPECT_TRUE(grid_->isHttp3Confirmed()); + EXPECT_LOG_CONTAINS("trace", "http3 pool successfully connected to host 'hostname'", + grid_->callbacks(0)->onPoolReady(encoder_, host_, info_, absl::nullopt)); + + grid_->removeCallbacks(); + + // Create a new stream now that we have an updated HTTP/3 status. + EXPECT_LOG_CONTAINS("trace", "http3 pool attempting to create a new stream to host 'hostname'", + grid_->newStream(decoder_, callbacks_, + {/*can_send_early_data=*/false, + /*can_use_http3_=*/true})); + + // Because HTTP/3 is confirmed, we skip attempting a TCP connection. + ASSERT_EQ(grid_->callbackSize(), 1); + grid_->onHandshakeComplete(); + EXPECT_TRUE(grid_->isHttp3Confirmed()); + EXPECT_CALL(callbacks_.pool_ready_, ready()); + EXPECT_LOG_CONTAINS("trace", "http3 pool successfully connected to host 'hostname'", + grid_->callbacks(0)->onPoolReady(encoder_, host_, info_, absl::nullopt)); +} + +TEST_F(ConnectivityGridTest, ParallelConnectionsNoTcpDelayQuicFailsTcpSucceeds) { + TestScopedRuntime scoped_runtime; + scoped_runtime.mergeValues({{"envoy.reloadable_features.quic_no_tcp_delay", "true"}}); + + initialize(); + grid_->alternate_immediate_ = false; + addHttp3AlternateProtocol(); + EXPECT_EQ(grid_->http3Pool(), nullptr); + + EXPECT_LOG_CONTAINS_ALL_OF( + Envoy::ExpectedLogMessages( + {{"trace", "http3 pool attempting to create a new stream to host 'hostname'"}, + {"trace", "http2 pool attempting to create a new stream to host 'hostname'"}}), + { + grid_->newStream(decoder_, callbacks_, + {/*can_send_early_data=*/false, + /*can_use_http3_=*/true}); + }); + + EXPECT_NE(grid_->http3Pool(), nullptr); + EXPECT_NE(grid_->http2Pool(), nullptr); + // Contains QUIC and TCP. + ASSERT_EQ(grid_->callbackSize(), 2); + + // Force QUIC to fail. + EXPECT_LOG_CONTAINS( + "trace", "http3 pool failed to create connection to host 'hostname'", + grid_->callbacks(0)->onPoolFailure(ConnectionPool::PoolFailureReason::LocalConnectionFailure, + "reason", host_)); + // HTTP/3 is still not marked broken yet. + EXPECT_FALSE(grid_->isHttp3Broken()); + // Also force the alternate QUIC to fail. + EXPECT_LOG_CONTAINS( + "trace", "alternate pool failed to create connection to host 'hostname'", + grid_->callbacks(2)->onPoolFailure(ConnectionPool::PoolFailureReason::LocalConnectionFailure, + "reason", host_)); + // HTTP/3 is still not marked broken yet. + EXPECT_FALSE(grid_->isHttp3Broken()); + + // TCP succeeds. + EXPECT_LOG_CONTAINS("trace", "http2 pool successfully connected to host 'hostname'", + grid_->callbacks(1)->onPoolReady(encoder_, host_, info_, absl::nullopt)); + // HTTP/3 is now marked broken because HTTP/3 failed and TCP succeeded. + EXPECT_TRUE(grid_->isHttp3Broken()); + + grid_->removeCallbacks(); + + // Create a new stream now that we have a broken HTTP/3 status. + EXPECT_LOG_CONTAINS("trace", "http2 pool attempting to create a new stream to host 'hostname'", + grid_->newStream(decoder_, callbacks_, + {/*can_send_early_data=*/false, + /*can_use_http3_=*/true})); + + // Because HTTP/3 is broken, we skip QUIC, so there is only one callback for TCP. + ASSERT_EQ(grid_->callbackSize(), 1); + EXPECT_CALL(callbacks_.pool_ready_, ready()); + EXPECT_LOG_CONTAINS("trace", "http2 pool successfully connected to host 'hostname'", + grid_->callbacks(0)->onPoolReady(encoder_, host_, info_, absl::nullopt)); +} + +TEST_F(ConnectivityGridTest, ParallelConnectionsNoTcpDelayTcpAndQuicFail) { + TestScopedRuntime scoped_runtime; + scoped_runtime.mergeValues({{"envoy.reloadable_features.quic_no_tcp_delay", "true"}}); + + initialize(); + grid_->alternate_immediate_ = false; + addHttp3AlternateProtocol(); + EXPECT_EQ(grid_->http3Pool(), nullptr); + + EXPECT_LOG_CONTAINS_ALL_OF( + Envoy::ExpectedLogMessages( + {{"trace", "http3 pool attempting to create a new stream to host 'hostname'"}, + {"trace", "http2 pool attempting to create a new stream to host 'hostname'"}}), + { + grid_->newStream(decoder_, callbacks_, + {/*can_send_early_data=*/false, + /*can_use_http3_=*/true}); + }); + + EXPECT_NE(grid_->http3Pool(), nullptr); + EXPECT_NE(grid_->http2Pool(), nullptr); + // Contains QUIC and TCP. + ASSERT_EQ(grid_->callbackSize(), 2); + + // Force QUIC to fail. + EXPECT_LOG_CONTAINS( + "trace", "http3 pool failed to create connection to host 'hostname'", + grid_->callbacks(0)->onPoolFailure(ConnectionPool::PoolFailureReason::LocalConnectionFailure, + "reason", host_)); + // HTTP/3 is still not marked broken yet. + EXPECT_FALSE(grid_->isHttp3Broken()); + // Also force the alternate QUIC to fail. + EXPECT_LOG_CONTAINS( + "trace", "alternate pool failed to create connection to host 'hostname'", + grid_->callbacks(2)->onPoolFailure(ConnectionPool::PoolFailureReason::LocalConnectionFailure, + "reason", host_)); + // HTTP/3 is still not marked broken yet. + EXPECT_FALSE(grid_->isHttp3Broken()); + + // Force TCP to fail. + EXPECT_LOG_CONTAINS( + "trace", "http2 pool failed to create connection to host 'hostname'", + grid_->callbacks(1)->onPoolFailure(ConnectionPool::PoolFailureReason::LocalConnectionFailure, + "reason", host_)); + // Because TCP failed, HTTP/3 is not marked broken. + EXPECT_FALSE(grid_->isHttp3Broken()); + + grid_->removeCallbacks(); + + // Because HTTP/3 status is neither marked confirmed nor broken, we will attempt both HTTP/3 and + // HTTP/2. + EXPECT_LOG_CONTAINS_ALL_OF( + Envoy::ExpectedLogMessages( + {{"trace", "http3 pool attempting to create a new stream to host 'hostname'"}, + {"trace", "http2 pool attempting to create a new stream to host 'hostname'"}}), + { + grid_->newStream(decoder_, callbacks_, + {/*can_send_early_data=*/false, + /*can_use_http3_=*/true}); + }); +} + +TEST_F(ConnectivityGridTest, ParallelConnectionsNoTcpDelayTcpFailsQuicSucceeds) { + TestScopedRuntime scoped_runtime; + scoped_runtime.mergeValues({{"envoy.reloadable_features.quic_no_tcp_delay", "true"}}); + + initialize(); + grid_->alternate_immediate_ = false; + addHttp3AlternateProtocol(); + EXPECT_EQ(grid_->http3Pool(), nullptr); + + EXPECT_LOG_CONTAINS_ALL_OF( + Envoy::ExpectedLogMessages( + {{"trace", "http3 pool attempting to create a new stream to host 'hostname'"}, + {"trace", "http2 pool attempting to create a new stream to host 'hostname'"}}), + { + grid_->newStream(decoder_, callbacks_, + {/*can_send_early_data=*/false, + /*can_use_http3_=*/true}); + }); + + EXPECT_NE(grid_->http3Pool(), nullptr); + EXPECT_NE(grid_->http2Pool(), nullptr); + // Contains QUIC and TCP. + ASSERT_EQ(grid_->callbackSize(), 2); + + // Force TCP to fail. + EXPECT_LOG_CONTAINS( + "trace", "http2 pool failed to create connection to host 'hostname'", + grid_->callbacks(1)->onPoolFailure(ConnectionPool::PoolFailureReason::LocalConnectionFailure, + "reason", host_)); + // QUIC succeeds. + grid_->onHandshakeComplete(); + EXPECT_TRUE(grid_->isHttp3Confirmed()); + EXPECT_CALL(callbacks_.pool_ready_, ready()); + EXPECT_LOG_CONTAINS("trace", "http3 pool successfully connected to host 'hostname'", + grid_->callbacks(0)->onPoolReady(encoder_, host_, info_, absl::nullopt)); + EXPECT_TRUE(grid_->isHttp3Confirmed()); + + grid_->removeCallbacks(); + + // Create a new stream now that we have a confirmed HTTP/3 status. + EXPECT_LOG_CONTAINS("trace", "http3 pool attempting to create a new stream to host 'hostname'", + grid_->newStream(decoder_, callbacks_, + {/*can_send_early_data=*/false, + /*can_use_http3_=*/true})); + + // Because HTTP/3 is confirmed, we skip TCP, so there is only one callback for QUIC. + ASSERT_EQ(grid_->callbackSize(), 1); + EXPECT_CALL(callbacks_.pool_ready_, ready()); + EXPECT_LOG_CONTAINS("trace", "http3 pool successfully connected to host 'hostname'", + grid_->callbacks(0)->onPoolReady(encoder_, host_, info_, absl::nullopt)); +} + // Same test as above but with the H3 alternate pool succeeding inline no TCP is attempted. TEST_F(ConnectivityGridTest, ParallelH3NoTcp) { initialize(); diff --git a/test/common/http/http3_status_tracker_impl_test.cc b/test/common/http/http3_status_tracker_impl_test.cc index 19c6a582f090..2949d12c1523 100644 --- a/test/common/http/http3_status_tracker_impl_test.cc +++ b/test/common/http/http3_status_tracker_impl_test.cc @@ -173,6 +173,23 @@ TEST_F(Http3StatusTrackerImplTest, MarkFailedRecentlyAndThenBroken) { EXPECT_FALSE(tracker_.hasHttp3FailedRecently()); } +TEST_F(Http3StatusTrackerImplTest, MarkPendingAndThenBroken) { + tracker_.markHttp3Pending(); + EXPECT_TRUE(tracker_.isHttp3Pending()); + EXPECT_FALSE(tracker_.isHttp3Broken()); + EXPECT_FALSE(tracker_.isHttp3Confirmed()); + EXPECT_FALSE(tracker_.hasHttp3FailedRecently()); + + EXPECT_CALL(*timer_, enabled()).WillOnce(Return(false)); + EXPECT_CALL(*timer_, enableTimer(std::chrono::milliseconds(1 * 1000), nullptr)); + tracker_.markHttp3Broken(); + + EXPECT_TRUE(tracker_.isHttp3Broken()); + EXPECT_FALSE(tracker_.isHttp3Pending()); + EXPECT_FALSE(tracker_.isHttp3Confirmed()); + EXPECT_FALSE(tracker_.hasHttp3FailedRecently()); +} + } // namespace } // namespace Http } // namespace Envoy diff --git a/test/mocks/http/http_server_properties_cache.h b/test/mocks/http/http_server_properties_cache.h index d29ed5ce1cb5..ef204f57794f 100644 --- a/test/mocks/http/http_server_properties_cache.h +++ b/test/mocks/http/http_server_properties_cache.h @@ -22,6 +22,7 @@ class MockHttpServerPropertiesCache : public HttpServerPropertiesCache { MOCK_METHOD(HttpServerPropertiesCache::Http3StatusTracker&, getOrCreateHttp3StatusTracker, (const Origin& origin)); MOCK_METHOD(void, resetBrokenness, ()); + MOCK_METHOD(void, resetStatus, ()); }; class MockHttpServerPropertiesCacheManager : public HttpServerPropertiesCacheManager {