quic: Don't delay TCP attempt when HTTP/3 status is unknown (envoypro…

…xy#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 <fredyw@google.com>
fredyw · Nov 8, 2024 · 232c19e · 232c19e
1 parent c414d28
commit 232c19e
Show file tree

Hide file tree

Showing 14 changed files with 361 additions and 7 deletions.
diff --git 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<HttpServerPropertiesCache>;

diff --git 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
@@ -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
@@ -18,10 +18,14 @@ TEST(FetchClientTest, Http2) {
 TEST(FetchClientTest, Http3) {
   Envoy::Fetch client;
   std::vector<Http::Protocol> 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
@@ -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
@@ -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
@@ -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
@@ -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
@@ -304,6 +304,14 @@ void HttpServerPropertiesCacheImpl::resetBrokenness() {
   }
 }
 
+void HttpServerPropertiesCacheImpl::resetStatus() {
+  for (const std::pair<Origin, OriginData>& 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
@@ -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
@@ -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);