From 0388bce6340ced5de761690704145ab81af012d6 Mon Sep 17 00:00:00 2001 From: Matt Klein Date: Thu, 28 Apr 2022 19:00:28 +0000 Subject: [PATCH 1/2] http/2: extend keep alive timeout when frame is received To avoid HOL blocking impacts on slow connections. Signed-off-by: Matt Klein --- api/envoy/config/core/v3/protocol.proto | 4 +- docs/root/version_history/current.rst | 6 ++ source/common/http/http2/codec_impl.cc | 15 ++++- source/common/http/http2/codec_impl.h | 1 + source/common/runtime/runtime_features.cc | 1 + test/common/http/http2/codec_impl_test.cc | 79 +++++++++++++++++++++++ 6 files changed, 104 insertions(+), 2 deletions(-) diff --git a/api/envoy/config/core/v3/protocol.proto b/api/envoy/config/core/v3/protocol.proto index ad22077281f83..f18a2053d9d3a 100644 --- a/api/envoy/config/core/v3/protocol.proto +++ b/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} diff --git a/docs/root/version_history/current.rst b/docs/root/version_history/current.rst index 3c04f3baa7c93..8e38ea6cba543 100644 --- a/docs/root/version_history/current.rst +++ b/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 ` + 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 ` to override the default behavior of cluster validation. * tls: if both :ref:`match_subject_alt_names ` and :ref:`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. diff --git a/source/common/http/http2/codec_impl.cc b/source/common/http/http2/codec_impl.cc index 952e564bd77c0..435bd19c26657 100644 --- a/source/common/http/http2/codec_impl.cc +++ b/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)); } diff --git a/source/common/http/http2/codec_impl.h b/source/common/http/http2/codec_impl.h index 5f984d7fe3453..2e5f8eb760e1c 100644 --- a/source/common/http/http2/codec_impl.h +++ b/source/common/http/http2/codec_impl.h @@ -700,6 +700,7 @@ class ConnectionImpl : public virtual Connection, std::map 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_{}; diff --git a/source/common/runtime/runtime_features.cc b/source/common/runtime/runtime_features.cc index 9e46458a0a7b8..7a31f1ccdd479 100644 --- a/source/common/runtime/runtime_features.cc +++ b/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); diff --git a/test/common/http/http2/codec_impl_test.cc b/test/common/http/http2/codec_impl_test.cc index 9676adaab5427..2ebe9439bffa1 100644 --- a/test/common/http/http2/codec_impl_test.cc +++ b/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(&client_connection_.dispatcher_); + auto send_timer = new NiceMock(&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(&client_connection_.dispatcher_); + auto send_timer = new NiceMock(&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); From 625a81d5ec4434d0cfb13b6a39eae8bb75b40dc9 Mon Sep 17 00:00:00 2001 From: Matt Klein Date: Thu, 28 Apr 2022 20:15:40 +0000 Subject: [PATCH 2/2] fix Signed-off-by: Matt Klein --- tools/spelling/spelling_dictionary.txt | 1 + 1 file changed, 1 insertion(+) diff --git a/tools/spelling/spelling_dictionary.txt b/tools/spelling/spelling_dictionary.txt index 11f39ffe7fdd2..a9420e8f5f22a 100644 --- a/tools/spelling/spelling_dictionary.txt +++ b/tools/spelling/spelling_dictionary.txt @@ -170,6 +170,7 @@ HC HCM HDS HMAC +HOL HPACK HTAB HTML