http/2: extend keep alive timeout when frame is received

api/envoy/config/core/v3/protocol.proto

@@ -336,7 +336,9 @@ message KeepaliveSettings {
   google.protobuf.Duration interval = 1 [(validate.rules).duration = {gte {nanos: 1000000}}];
 
   // How long to wait for a response to a keepalive PING. If a response is not received within this
-  // time period, the connection will be aborted.
+  // time period, the connection will be aborted. Note that in order to prevent the influence of
+  // Head-of-line (HOL) blocking the timeout period is extended when *any* frame is received on
+  // the connection, under the assumption that if a frame is received the connection is healthy.
   google.protobuf.Duration timeout = 2 [(validate.rules).duration = {
     required: true
     gte {nanos: 1000000}

docs/root/version_history/current.rst

@@ -11,6 +11,12 @@ Minor Behavior Changes
 ----------------------
 *Changes that may cause incompatibilities for some users, but should not for most*
 
+* http: the behavior of the :ref:`timeout <envoy_v3_api_field_config.core.v3.KeepaliveSettings.timeout>`
+  field has been modified to extend the timeout when *any* frame is received on the owning HTTP/2
+  connection. This negates the effect of head-of-line (HOL) blocking for slow connections. If
+  any frame is received the assumption is that the connection is working. This behavior change
+  can be reverted by setting the ``envoy.reloadable_features.http2_delay_keepalive_timeout`` runtime
+  flag to false.
 * thrift: add validate_clusters in :ref:`RouteConfiguration <envoy_v3_api_msg_extensions.filters.network.thrift_proxy.v3.RouteConfiguration>` to override the default behavior of cluster validation.
 * tls: if both :ref:`match_subject_alt_names <envoy_v3_api_field_extensions.transport_sockets.tls.v3.CertificateValidationContext.match_subject_alt_names>` and :ref:`match_typed_subject_alt_names <envoy_v3_api_field_extensions.transport_sockets.tls.v3.CertificateValidationContext.match_typed_subject_alt_names>` are specified, the former (deprecated) field is ignored. Previously, setting both fields would result in an error.
 * tls: removed SHA-1 cipher suites from the server-side defaults.

source/common/http/http2/codec_impl.cc

@@ -907,7 +907,10 @@ ConnectionImpl::ConnectionImpl(Network::Connection& connection, CodecStats& stat
       protocol_constraints_(stats, http2_options),
       skip_dispatching_frames_for_closed_connection_(Runtime::runtimeFeatureEnabled(
           "envoy.reloadable_features.skip_dispatching_frames_for_closed_connection")),
-      dispatching_(false), raised_goaway_(false), random_(random_generator),
+      dispatching_(false), raised_goaway_(false),
+      delay_keepalive_timeout_(Runtime::runtimeFeatureEnabled(
+          "envoy.reloadable_features.http2_delay_keepalive_timeout")),
+      random_(random_generator),
       last_received_data_time_(connection_.dispatcher().timeSource().monotonicTime()) {
   // This library can only be used with the wrapper API enabled.
   ASSERT(!use_oghttp2_library_ || use_new_codec_wrapper_);
@@ -1181,6 +1184,16 @@ Status ConnectionImpl::onFrameReceived(const nghttp2_frame* frame) {
     return okStatus();
   }
 
+  // In slow networks, HOL blocking can prevent the ping response from coming in a reasonable
+  // amount of time. To avoid HOL blocking influence, if we receive *any* frame extend the
+  // timeout for another timeout period. This will still timeout the connection if there is no
+  // activity, but if there is frame activity we assume the connection is still healthy and the
+  // PING ACK may be delayed behind other frames.
+  if (delay_keepalive_timeout_ && keepalive_timeout_timer_ != nullptr &&
+      keepalive_timeout_timer_->enabled()) {
+    keepalive_timeout_timer_->enableTimer(keepalive_timeout_);
+  }
+
   if (frame->hd.type == NGHTTP2_DATA) {
     RETURN_IF_ERROR(trackInboundFrames(&frame->hd, frame->data.padlen));
   }

source/common/http/http2/codec_impl.h

@@ -700,6 +700,7 @@ class ConnectionImpl : public virtual Connection,
   std::map<int32_t, StreamImpl*> pending_deferred_reset_streams_;
   bool dispatching_ : 1;
   bool raised_goaway_ : 1;
+  const bool delay_keepalive_timeout_ : 1;
   Event::SchedulableCallbackPtr protocol_constraint_violation_callback_;
   Random::RandomGenerator& random_;
   MonotonicTime last_received_data_time_{};

source/common/runtime/runtime_features.cc

@@ -42,6 +42,7 @@ RUNTIME_GUARD(envoy_reloadable_features_enable_grpc_async_client_cache);
 RUNTIME_GUARD(envoy_reloadable_features_handle_stream_reset_during_hcm_encoding);
 RUNTIME_GUARD(envoy_reloadable_features_http1_lazy_read_disable);
 RUNTIME_GUARD(envoy_reloadable_features_http2_allow_capacity_increase_by_settings);
+RUNTIME_GUARD(envoy_reloadable_features_http2_delay_keepalive_timeout);
 RUNTIME_GUARD(envoy_reloadable_features_http2_new_codec_wrapper);
 RUNTIME_GUARD(envoy_reloadable_features_http_100_continue_case_insensitive);
 RUNTIME_GUARD(envoy_reloadable_features_http_reject_path_with_fragment);

test/common/http/http2/codec_impl_test.cc

@@ -1184,6 +1184,85 @@ TEST_P(Http2CodecImplTest, ConnectionKeepalive) {
   timeout_timer->invokeCallback();
 }
 
+// Verify that extending the timeout is performed when a frame is received.
+TEST_P(Http2CodecImplTest, KeepaliveTimeoutDelay) {
+  constexpr uint32_t interval_ms = 100;
+  constexpr uint32_t timeout_ms = 200;
+  client_http2_options_.mutable_connection_keepalive()->mutable_interval()->set_nanos(interval_ms *
+                                                                                      1000 * 1000);
+  client_http2_options_.mutable_connection_keepalive()->mutable_timeout()->set_nanos(timeout_ms *
+                                                                                     1000 * 1000);
+  client_http2_options_.mutable_connection_keepalive()->mutable_interval_jitter()->set_value(0);
+  auto timeout_timer = new NiceMock<Event::MockTimer>(&client_connection_.dispatcher_);
+  auto send_timer = new NiceMock<Event::MockTimer>(&client_connection_.dispatcher_);
+  EXPECT_CALL(*timeout_timer, disableTimer());
+  EXPECT_CALL(*send_timer, enableTimer(std::chrono::milliseconds(interval_ms), _));
+  initialize();
+  InSequence s;
+
+  // Initiate a request.
+  TestRequestHeaderMapImpl request_headers;
+  HttpTestUtility::addDefaultHeaders(request_headers);
+  EXPECT_CALL(request_decoder_, decodeHeaders_(_, true));
+  EXPECT_TRUE(request_encoder_->encodeHeaders(request_headers, true).ok());
+  driveToCompletion();
+
+  // Now send a ping.
+  EXPECT_CALL(*timeout_timer, enableTimer(std::chrono::milliseconds(timeout_ms), _));
+  send_timer->invokeCallback();
+
+  // Send the response and make sure the keepalive timeout is extended. After the response is
+  // received the ACK will come in and reset.
+  EXPECT_CALL(*timeout_timer, enableTimer(std::chrono::milliseconds(timeout_ms), _));
+  EXPECT_CALL(response_decoder_, decodeHeaders_(_, true));
+  EXPECT_CALL(*timeout_timer, disableTimer()); // This indicates that an ACK was received.
+  EXPECT_CALL(*send_timer, enableTimer(std::chrono::milliseconds(interval_ms), _));
+  TestResponseHeaderMapImpl response_headers{{":status", "200"}};
+  response_encoder_->encodeHeaders(response_headers, true);
+  driveToCompletion();
+}
+
+// Verify that extending the timeout is not performed when a frame is received and the feature
+// flag is disabled.
+TEST_P(Http2CodecImplTest, KeepaliveTimeoutDelayRuntimeFalse) {
+  scoped_runtime_.mergeValues(
+      {{"envoy.reloadable_features.http2_delay_keepalive_timeout", "false"}});
+
+  constexpr uint32_t interval_ms = 100;
+  constexpr uint32_t timeout_ms = 200;
+  client_http2_options_.mutable_connection_keepalive()->mutable_interval()->set_nanos(interval_ms *
+                                                                                      1000 * 1000);
+  client_http2_options_.mutable_connection_keepalive()->mutable_timeout()->set_nanos(timeout_ms *
+                                                                                     1000 * 1000);
+  client_http2_options_.mutable_connection_keepalive()->mutable_interval_jitter()->set_value(0);
+  auto timeout_timer = new NiceMock<Event::MockTimer>(&client_connection_.dispatcher_);
+  auto send_timer = new NiceMock<Event::MockTimer>(&client_connection_.dispatcher_);
+  EXPECT_CALL(*timeout_timer, disableTimer());
+  EXPECT_CALL(*send_timer, enableTimer(std::chrono::milliseconds(interval_ms), _));
+  initialize();
+  InSequence s;
+
+  // Initiate a request.
+  TestRequestHeaderMapImpl request_headers;
+  HttpTestUtility::addDefaultHeaders(request_headers);
+  EXPECT_CALL(request_decoder_, decodeHeaders_(_, true));
+  EXPECT_TRUE(request_encoder_->encodeHeaders(request_headers, true).ok());
+  driveToCompletion();
+
+  // Now send a ping.
+  EXPECT_CALL(*timeout_timer, enableTimer(std::chrono::milliseconds(timeout_ms), _));
+  send_timer->invokeCallback();
+
+  // Send the response and make sure the keepalive timeout is not extended. After the response is
+  // received the ACK will come in and reset.
+  EXPECT_CALL(response_decoder_, decodeHeaders_(_, true));
+  EXPECT_CALL(*timeout_timer, disableTimer()); // This indicates that an ACK was received.
+  EXPECT_CALL(*send_timer, enableTimer(std::chrono::milliseconds(interval_ms), _));
+  TestResponseHeaderMapImpl response_headers{{":status", "200"}};
+  response_encoder_->encodeHeaders(response_headers, true);
+  driveToCompletion();
+}
+
 // Validate that jitter is added as expected based on configuration.
 TEST_P(Http2CodecImplTest, ConnectionKeepaliveJitter) {
   client_http2_options_.mutable_connection_keepalive()->mutable_interval()->set_seconds(1);