From 4d2426e54f6219b81a44c455635fb97d710d837c Mon Sep 17 00:00:00 2001 From: Andres Guedez Date: Fri, 31 Aug 2018 14:01:02 -0400 Subject: [PATCH 01/19] network: Support delayed close of downstream conns (#2929) Mitigate client read/close race issues on downstream HTTP connections by adding a new connection close type 'FlushWriteAndDelay'. This new close type flushes the write buffer on a connection but does not immediately close after emptying the buffer (unlike ConnectionCloseType::FlushWrite). A timer has been added to track delayed closes for both 'FlushWrite' and 'FlushWriteAndDelay'. Upon triggering, the socket will be closed and the connection will be cleaned up. Delayed close processing can be disabled by setting the newly added HCM 'delayed_close_timeout' config option to 0. Risk Level: Medium (changes common case behavior for closing of downstream HTTP connections) Testing: Unit tests and integration tests added. Fixes #2929. Signed-off-by: Andres Guedez --- .../v2/http_connection_manager.proto | 13 +- include/envoy/network/connection.h | 17 +- source/common/http/conn_manager_config.h | 1 + source/common/http/conn_manager_impl.cc | 14 +- source/common/http/http1/codec_impl.cc | 7 +- source/common/http/http1/codec_impl.h | 5 +- source/common/http/http1/conn_pool.cc | 2 +- source/common/http/http2/codec_impl.cc | 4 +- source/common/http/http2/codec_impl.h | 6 +- source/common/http/http2/conn_pool.cc | 2 +- source/common/network/connection_impl.cc | 42 +++- source/common/network/connection_impl.h | 13 +- source/common/tcp/conn_pool.cc | 2 +- source/common/tcp_proxy/tcp_proxy.cc | 2 +- .../network/http_connection_manager/config.cc | 15 +- .../network/http_connection_manager/config.h | 2 + .../network/redis_proxy/proxy_filter.cc | 2 +- .../stat_sinks/common/statsd/statsd.cc | 2 +- test/common/http/conn_manager_impl_test.cc | 19 +- test/common/network/connection_impl_test.cc | 186 +++++++++++++++++- test/integration/http_integration.cc | 20 +- test/integration/http_integration.h | 4 +- test/integration/integration_test.cc | 107 ++++++++++ test/mocks/network/mocks.h | 5 +- 24 files changed, 446 insertions(+), 46 deletions(-) diff --git a/api/envoy/config/filter/network/http_connection_manager/v2/http_connection_manager.proto b/api/envoy/config/filter/network/http_connection_manager/v2/http_connection_manager.proto index 1e6999cf2077a..768d0999c333e 100644 --- a/api/envoy/config/filter/network/http_connection_manager/v2/http_connection_manager.proto +++ b/api/envoy/config/filter/network/http_connection_manager/v2/http_connection_manager.proto @@ -19,7 +19,7 @@ import "gogoproto/gogo.proto"; // [#protodoc-title: HTTP connection manager] // HTTP connection manager :ref:`configuration overview `. -// [#comment:next free field: 25] +// [#comment:next free field: 26] message HttpConnectionManager { enum CodecType { option (gogoproto.goproto_enum_prefix) = false; @@ -175,6 +175,17 @@ message HttpConnectionManager { // option is not specified. google.protobuf.Duration drain_timeout = 12 [(gogoproto.stdduration) = true]; + // The delayed close timeout is for downstream connections managed by the HTTP connection manager. + // It is defined as a grace period after connection close processing has been locally initiated + // during which Envoy will flush the write buffers for the connection and await the peer to close + // the connection (i.e., issue a TCP FIN/RST). + // + // If the timeout triggers, Envoy will close the connection's socket. + // + // A value of 0 will completely disable delayed close processing, and the downstream connection's + // socket will be closed immediately after the write flush is completed. + google.protobuf.Duration delayed_close_timeout = 25 [(gogoproto.stdduration) = true]; + // Configuration for :ref:`HTTP access logs ` // emitted by the connection manager. repeated envoy.config.filter.accesslog.v2.AccessLog access_log = 13; diff --git a/include/envoy/network/connection.h b/include/envoy/network/connection.h index 7574f3686b255..9b805546f34d4 100644 --- a/include/envoy/network/connection.h +++ b/include/envoy/network/connection.h @@ -63,7 +63,8 @@ class ConnectionCallbacks { */ enum class ConnectionCloseType { FlushWrite, // Flush pending write data before raising ConnectionEvent::LocalClose - NoFlush // Do not flush any pending data and immediately raise ConnectionEvent::LocalClose + NoFlush, // Do not flush any pending data and immediately raise ConnectionEvent::LocalClose + FlushWriteAndDelay }; /** @@ -86,6 +87,8 @@ class Connection : public Event::DeferredDeletable, public FilterManager { Stats::Gauge& write_current_; // Counter* as this is an optional counter. Bind errors will not be tracked if this is nullptr. Stats::Counter* bind_errors_; + // Optional counter. Delayed close semantics are only used by HTTP connections. + Stats::Counter* delayed_close_timeouts_; }; virtual ~Connection() {} @@ -233,6 +236,18 @@ class Connection : public Event::DeferredDeletable, public FilterManager { * Get the socket options set on this connection. */ virtual const ConnectionSocket::OptionsSharedPtr& socketOptions() const PURE; + + /** + * Set the timeout for delayed connection close()s. + * This is only used for downstream connections processing HTTP/1 and HTTP/2. + * @param timeout The timeout value in milliseconds + */ + virtual void setDelayedCloseTimeout(std::chrono::milliseconds timeout) PURE; + + /** + * @return std::chrono::milliseconds The delayed close timeout value. + */ + virtual std::chrono::milliseconds delayedCloseTimeout() const PURE; }; typedef std::unique_ptr ConnectionPtr; diff --git a/source/common/http/conn_manager_config.h b/source/common/http/conn_manager_config.h index a1dae7a55e2a3..7d4cd07838ff9 100644 --- a/source/common/http/conn_manager_config.h +++ b/source/common/http/conn_manager_config.h @@ -37,6 +37,7 @@ namespace Http { GAUGE (downstream_cx_tx_bytes_buffered) \ COUNTER (downstream_cx_drain_close) \ COUNTER (downstream_cx_idle_timeout) \ + COUNTER (downstream_cx_delayed_close_timeout) \ COUNTER (downstream_flow_control_paused_reading_total) \ COUNTER (downstream_flow_control_resumed_reading_total) \ COUNTER (downstream_rq_total) \ diff --git a/source/common/http/conn_manager_impl.cc b/source/common/http/conn_manager_impl.cc index 3c93fb83939d0..0ac83e0eadcf6 100644 --- a/source/common/http/conn_manager_impl.cc +++ b/source/common/http/conn_manager_impl.cc @@ -90,7 +90,7 @@ void ConnectionManagerImpl::initializeReadFilterCallbacks(Network::ReadFilterCal read_callbacks_->connection().setConnectionStats( {stats_.named_.downstream_cx_rx_bytes_total_, stats_.named_.downstream_cx_rx_bytes_buffered_, stats_.named_.downstream_cx_tx_bytes_total_, stats_.named_.downstream_cx_tx_bytes_buffered_, - nullptr}); + nullptr, &stats_.named_.downstream_cx_delayed_close_timeout_}); } ConnectionManagerImpl::~ConnectionManagerImpl() { @@ -118,7 +118,7 @@ ConnectionManagerImpl::~ConnectionManagerImpl() { void ConnectionManagerImpl::checkForDeferredClose() { if (drain_state_ == DrainState::Closing && streams_.empty() && !codec_->wantsToWrite()) { - read_callbacks_->connection().close(Network::ConnectionCloseType::FlushWrite); + read_callbacks_->connection().close(Network::ConnectionCloseType::FlushWriteAndDelay); } } @@ -233,12 +233,12 @@ Network::FilterStatus ConnectionManagerImpl::onData(Buffer::Instance& data, bool ENVOY_CONN_LOG(debug, "dispatch error: {}", read_callbacks_->connection(), e.what()); stats_.named_.downstream_cx_protocol_error_.inc(); - // In the protocol error case, we need to reset all streams now. Since we do a flush write, - // the connection might stick around long enough for a pending stream to come back and try - // to encode. + // In the protocol error case, we need to reset all streams now. Since we do a flush write and + // delayed close, the connection might stick around long enough for a pending stream to come + // back and try to encode. resetAllStreams(); - read_callbacks_->connection().close(Network::ConnectionCloseType::FlushWrite); + read_callbacks_->connection().close(Network::ConnectionCloseType::FlushWriteAndDelay); return Network::FilterStatus::StopIteration; } @@ -317,6 +317,8 @@ void ConnectionManagerImpl::onIdleTimeout() { ENVOY_CONN_LOG(debug, "idle timeout", read_callbacks_->connection()); stats_.named_.downstream_cx_idle_timeout_.inc(); if (!codec_) { + // No need to delay close after flushing since an idle timeout has already fired. Attempt to + // write out buffered data one last time and issue a local close if successful. read_callbacks_->connection().close(Network::ConnectionCloseType::FlushWrite); } else if (drain_state_ == DrainState::NotDraining) { startDrainSequence(); diff --git a/source/common/http/http1/codec_impl.cc b/source/common/http/http1/codec_impl.cc index 24d9d6d49c3e1..0dbadaa47c92d 100644 --- a/source/common/http/http1/codec_impl.cc +++ b/source/common/http/http1/codec_impl.cc @@ -450,8 +450,11 @@ void ConnectionImpl::onResetStreamBase(StreamResetReason reason) { ServerConnectionImpl::ServerConnectionImpl(Network::Connection& connection, ServerConnectionCallbacks& callbacks, - Http1Settings settings) - : ConnectionImpl(connection, HTTP_REQUEST), callbacks_(callbacks), codec_settings_(settings) {} + Http1Settings settings, + std::chrono::milliseconds delayed_close_timeout) + : ConnectionImpl(connection, HTTP_REQUEST), callbacks_(callbacks), codec_settings_(settings) { + connection_.setDelayedCloseTimeout(delayed_close_timeout); +} void ServerConnectionImpl::onEncodeComplete() { ASSERT(active_request_); diff --git a/source/common/http/http1/codec_impl.h b/source/common/http/http1/codec_impl.h index 6f8cafd2c9714..e974aa03187e1 100644 --- a/source/common/http/http1/codec_impl.h +++ b/source/common/http/http1/codec_impl.h @@ -277,8 +277,9 @@ class ConnectionImpl : public virtual Connection, protected Logger::Loggablecluster().stats().upstream_cx_rx_bytes_buffered_, parent_.host_->cluster().stats().upstream_cx_tx_bytes_total_, parent_.host_->cluster().stats().upstream_cx_tx_bytes_buffered_, - &parent_.host_->cluster().stats().bind_errors_}); + &parent_.host_->cluster().stats().bind_errors_, nullptr}); } ConnPoolImpl::ActiveClient::~ActiveClient() { diff --git a/source/common/http/http2/codec_impl.cc b/source/common/http/http2/codec_impl.cc index 9451e82e4d397..639153e73822a 100644 --- a/source/common/http/http2/codec_impl.cc +++ b/source/common/http/http2/codec_impl.cc @@ -812,8 +812,10 @@ int ClientConnectionImpl::onHeader(const nghttp2_frame* frame, HeaderString&& na ServerConnectionImpl::ServerConnectionImpl(Network::Connection& connection, Http::ServerConnectionCallbacks& callbacks, - Stats::Scope& scope, const Http2Settings& http2_settings) + Stats::Scope& scope, const Http2Settings& http2_settings, + std::chrono::milliseconds delayed_close_timeout) : ConnectionImpl(connection, scope, http2_settings), callbacks_(callbacks) { + connection_.setDelayedCloseTimeout(delayed_close_timeout); Http2Options http2_options(http2_settings); nghttp2_session_server_new2(&session_, http2_callbacks_.callbacks(), base(), http2_options.options()); diff --git a/source/common/http/http2/codec_impl.h b/source/common/http/http2/codec_impl.h index e5786a6670bc4..e813e3cdca17f 100644 --- a/source/common/http/http2/codec_impl.h +++ b/source/common/http/http2/codec_impl.h @@ -323,8 +323,10 @@ class ClientConnectionImpl : public ClientConnection, public ConnectionImpl { */ class ServerConnectionImpl : public ServerConnection, public ConnectionImpl { public: - ServerConnectionImpl(Network::Connection& connection, ServerConnectionCallbacks& callbacks, - Stats::Scope& scope, const Http2Settings& http2_settings); + ServerConnectionImpl( + Network::Connection& connection, ServerConnectionCallbacks& callbacks, Stats::Scope& scope, + const Http2Settings& http2_settings, + std::chrono::milliseconds delayed_close_timeout = std::chrono::milliseconds(0)); private: // ConnectionImpl diff --git a/source/common/http/http2/conn_pool.cc b/source/common/http/http2/conn_pool.cc index cd8f6c3f99abf..d4d591572d088 100644 --- a/source/common/http/http2/conn_pool.cc +++ b/source/common/http/http2/conn_pool.cc @@ -241,7 +241,7 @@ ConnPoolImpl::ActiveClient::ActiveClient(ConnPoolImpl& parent) parent_.host_->cluster().stats().upstream_cx_rx_bytes_buffered_, parent_.host_->cluster().stats().upstream_cx_tx_bytes_total_, parent_.host_->cluster().stats().upstream_cx_tx_bytes_buffered_, - &parent_.host_->cluster().stats().bind_errors_}); + &parent_.host_->cluster().stats().bind_errors_, nullptr}); } ConnPoolImpl::ActiveClient::~ActiveClient() { diff --git a/source/common/network/connection_impl.cc b/source/common/network/connection_impl.cc index 24652ab46039e..6a7c9fffd094f 100644 --- a/source/common/network/connection_impl.cc +++ b/source/common/network/connection_impl.cc @@ -70,6 +70,12 @@ ConnectionImpl::ConnectionImpl(Event::Dispatcher& dispatcher, ConnectionSocketPt ConnectionImpl::~ConnectionImpl() { ASSERT(fd() == -1, "ConnectionImpl was unexpectedly torn down without being closed."); + if (delayed_close_timer_) { + // It's ok to disable even if the timer has already fired. + delayed_close_timer_->disableTimer(); + delayed_close_timer_.reset(); + } + // In general we assume that owning code has called close() previously to the destructor being // run. This generally must be done so that callbacks run in the correct context (vs. deferred // deletion). Hence the assert above. However, call close() here just to be completely sure that @@ -106,10 +112,29 @@ void ConnectionImpl::close(ConnectionCloseType type) { closeSocket(ConnectionEvent::LocalClose); } else { - // TODO(mattklein123): We need a flush timer here. We might never get open socket window. - ASSERT(type == ConnectionCloseType::FlushWrite); - close_with_flush_ = true; + RELEASE_ASSERT(type == ConnectionCloseType::FlushWrite || + type == ConnectionCloseType::FlushWriteAndDelay, + ""); + delayed_close_ = true; + bool delayed_close_timeout_set = delayedCloseTimeout().count(); + + // All close types that follow do not actually close() the socket immediately so that buffered + // data can be written. However, we do want to stop reading to apply TCP backpressure. read_enabled_ = false; + + // Force a closeSocket() after the write buffer is flushed if the close_type calls for it or if + // no delayed close timeout is set. + close_after_flush_ = type == ConnectionCloseType::FlushWrite || !delayed_close_timeout_set; + + // Create and activate a timer which will immediately close the connection if triggered. + // A config value of 0 disables the timeout. + if (delayed_close_timeout_set) { + delayed_close_timer_ = dispatcher_.createTimer([this]() -> void { onDelayedCloseTimeout(); }); + ENVOY_CONN_LOG(debug, "setting delayed close timer with timeout {}", *this, + delayedCloseTimeout().count()); + delayed_close_timer_->enableTimer(delayedCloseTimeout()); + } + file_event_->setEnabled(Event::FileReadyType::Write | (enable_half_close_ ? 0 : Event::FileReadyType::Closed)); } @@ -118,7 +143,7 @@ void ConnectionImpl::close(ConnectionCloseType type) { Connection::State ConnectionImpl::state() const { if (fd() == -1) { return State::Closed; - } else if (close_with_flush_) { + } else if (delayed_close_) { return State::Closing; } else { return State::Open; @@ -488,7 +513,7 @@ void ConnectionImpl::onWriteReady() { // write callback. This can happen if we manage to complete the SSL handshake in the write // callback, raise a connected event, and close the connection. closeSocket(ConnectionEvent::RemoteClose); - } else if ((close_with_flush_ && new_buffer_size == 0) || bothSidesHalfClosed()) { + } else if ((close_after_flush_ && new_buffer_size == 0) || bothSidesHalfClosed()) { ENVOY_CONN_LOG(debug, "write flush complete", *this); closeSocket(ConnectionEvent::LocalClose); } else if (result.action_ == PostIoAction::KeepOpen && result.bytes_processed_ > 0) { @@ -535,6 +560,13 @@ bool ConnectionImpl::bothSidesHalfClosed() { return read_end_stream_ && write_end_stream_ && write_buffer_->length() == 0; } +void ConnectionImpl::onDelayedCloseTimeout() { + ENVOY_CONN_LOG(debug, "triggered delayed close", *this); + ASSERT(connection_stats_->delayed_close_timeouts_); + connection_stats_->delayed_close_timeouts_->inc(); + closeSocket(ConnectionEvent::LocalClose); +} + ClientConnectionImpl::ClientConnectionImpl( Event::Dispatcher& dispatcher, const Address::InstanceConstSharedPtr& remote_address, const Network::Address::InstanceConstSharedPtr& source_address, diff --git a/source/common/network/connection_impl.h b/source/common/network/connection_impl.h index 9bb12ef6bc42b..fb8d224bc78b1 100644 --- a/source/common/network/connection_impl.h +++ b/source/common/network/connection_impl.h @@ -114,6 +114,11 @@ class ConnectionImpl : public virtual Connection, // Obtain global next connection ID. This should only be used in tests. static uint64_t nextGlobalIdForTest() { return next_global_id_; } + void setDelayedCloseTimeout(std::chrono::milliseconds timeout) override { + delayed_close_timeout_ = timeout; + } + std::chrono::milliseconds delayedCloseTimeout() const override { return delayed_close_timeout_; } + protected: void closeSocket(ConnectionEvent close_type); @@ -128,6 +133,7 @@ class ConnectionImpl : public virtual Connection, // a generic pointer. Buffer::InstancePtr write_buffer_; uint32_t read_buffer_limit_ = 0; + std::chrono::milliseconds delayed_close_timeout_{0}; protected: bool connecting_{false}; @@ -146,14 +152,19 @@ class ConnectionImpl : public virtual Connection, // Returns true iff end of stream has been both written and read. bool bothSidesHalfClosed(); + // Callback issued when a delayed close timeout triggers. + void onDelayedCloseTimeout(); + static std::atomic next_global_id_; Event::Dispatcher& dispatcher_; const uint64_t id_; + Event::TimerPtr delayed_close_timer_; std::list callbacks_; std::list bytes_sent_callbacks_; bool read_enabled_{true}; - bool close_with_flush_{false}; + bool close_after_flush_{false}; + bool delayed_close_{false}; bool above_high_watermark_{false}; bool detect_early_close_{true}; bool enable_half_close_{false}; diff --git a/source/common/tcp/conn_pool.cc b/source/common/tcp/conn_pool.cc index 3204186b724ba..c39e460c61990 100644 --- a/source/common/tcp/conn_pool.cc +++ b/source/common/tcp/conn_pool.cc @@ -337,7 +337,7 @@ ConnPoolImpl::ActiveConn::ActiveConn(ConnPoolImpl& parent) parent_.host_->cluster().stats().upstream_cx_rx_bytes_buffered_, parent_.host_->cluster().stats().upstream_cx_tx_bytes_total_, parent_.host_->cluster().stats().upstream_cx_tx_bytes_buffered_, - &parent_.host_->cluster().stats().bind_errors_}); + &parent_.host_->cluster().stats().bind_errors_, nullptr}); // We just universally set no delay on connections. Theoretically we might at some point want // to make this configurable. diff --git a/source/common/tcp_proxy/tcp_proxy.cc b/source/common/tcp_proxy/tcp_proxy.cc index e4517f158111e..2c22d097a6c97 100644 --- a/source/common/tcp_proxy/tcp_proxy.cc +++ b/source/common/tcp_proxy/tcp_proxy.cc @@ -167,7 +167,7 @@ void Filter::initialize(Network::ReadFilterCallbacks& callbacks, bool set_connec {config_->stats().downstream_cx_rx_bytes_total_, config_->stats().downstream_cx_rx_bytes_buffered_, config_->stats().downstream_cx_tx_bytes_total_, - config_->stats().downstream_cx_tx_bytes_buffered_, nullptr}); + config_->stats().downstream_cx_tx_bytes_buffered_, nullptr, nullptr}); } } diff --git a/source/extensions/filters/network/http_connection_manager/config.cc b/source/extensions/filters/network/http_connection_manager/config.cc index feca899549d21..fa7a663b1aef5 100644 --- a/source/extensions/filters/network/http_connection_manager/config.cc +++ b/source/extensions/filters/network/http_connection_manager/config.cc @@ -136,7 +136,8 @@ HttpConnectionManagerConfig::HttpConnectionManagerConfig( date_provider_(date_provider), listener_stats_(Http::ConnectionManagerImpl::generateListenerStats(stats_prefix_, context_.listenerScope())), - proxy_100_continue_(config.proxy_100_continue()) { + proxy_100_continue_(config.proxy_100_continue()), + delayed_close_timeout_(PROTOBUF_GET_MS_OR_DEFAULT(config, delayed_close_timeout, 1000)) { route_config_provider_ = Router::RouteConfigProviderUtil::create(config, context_, stats_prefix_, route_config_provider_manager_); @@ -303,19 +304,19 @@ HttpConnectionManagerConfig::createCodec(Network::Connection& connection, Http::ServerConnectionCallbacks& callbacks) { switch (codec_type_) { case CodecType::HTTP1: - return Http::ServerConnectionPtr{ - new Http::Http1::ServerConnectionImpl(connection, callbacks, http1_settings_)}; + return Http::ServerConnectionPtr{new Http::Http1::ServerConnectionImpl( + connection, callbacks, http1_settings_, delayed_close_timeout_)}; case CodecType::HTTP2: return Http::ServerConnectionPtr{new Http::Http2::ServerConnectionImpl( - connection, callbacks, context_.scope(), http2_settings_)}; + connection, callbacks, context_.scope(), http2_settings_, delayed_close_timeout_)}; case CodecType::AUTO: if (HttpConnectionManagerConfigUtility::determineNextProtocol(connection, data) == Http::Http2::ALPN_STRING) { return Http::ServerConnectionPtr{new Http::Http2::ServerConnectionImpl( - connection, callbacks, context_.scope(), http2_settings_)}; + connection, callbacks, context_.scope(), http2_settings_, delayed_close_timeout_)}; } else { - return Http::ServerConnectionPtr{ - new Http::Http1::ServerConnectionImpl(connection, callbacks, http1_settings_)}; + return Http::ServerConnectionPtr{new Http::Http1::ServerConnectionImpl( + connection, callbacks, http1_settings_, delayed_close_timeout_)}; } } diff --git a/source/extensions/filters/network/http_connection_manager/config.h b/source/extensions/filters/network/http_connection_manager/config.h index c4cffd2dd458a..b8bf8fb8259f6 100644 --- a/source/extensions/filters/network/http_connection_manager/config.h +++ b/source/extensions/filters/network/http_connection_manager/config.h @@ -109,6 +109,7 @@ class HttpConnectionManagerConfig : Logger::Loggable, Http::ConnectionManagerListenerStats& listenerStats() override { return listener_stats_; } bool proxy100Continue() const override { return proxy_100_continue_; } const Http::Http1Settings& http1Settings() const override { return http1_settings_; } + std::chrono::milliseconds delayedCloseTimeout() const { return delayed_close_timeout_; } private: typedef std::list FilterFactoriesList; @@ -145,6 +146,7 @@ class HttpConnectionManagerConfig : Logger::Loggable, Http::DateProvider& date_provider_; Http::ConnectionManagerListenerStats listener_stats_; const bool proxy_100_continue_; + std::chrono::milliseconds delayed_close_timeout_; // Default idle timeout is 5 minutes if nothing is specified in the HCM config. static const uint64_t StreamIdleTimeoutMs = 5 * 60 * 1000; diff --git a/source/extensions/filters/network/redis_proxy/proxy_filter.cc b/source/extensions/filters/network/redis_proxy/proxy_filter.cc index e64fb795f3d47..00b07511922b5 100644 --- a/source/extensions/filters/network/redis_proxy/proxy_filter.cc +++ b/source/extensions/filters/network/redis_proxy/proxy_filter.cc @@ -49,7 +49,7 @@ void ProxyFilter::initializeReadFilterCallbacks(Network::ReadFilterCallbacks& ca config_->stats_.downstream_cx_rx_bytes_buffered_, config_->stats_.downstream_cx_tx_bytes_total_, config_->stats_.downstream_cx_tx_bytes_buffered_, - nullptr}); + nullptr, nullptr}); } void ProxyFilter::onRespValue(RespValuePtr&& value) { diff --git a/source/extensions/stat_sinks/common/statsd/statsd.cc b/source/extensions/stat_sinks/common/statsd/statsd.cc index 6882d10c68033..ba848e4aac26d 100644 --- a/source/extensions/stat_sinks/common/statsd/statsd.cc +++ b/source/extensions/stat_sinks/common/statsd/statsd.cc @@ -244,7 +244,7 @@ void TcpStatsdSink::TlsSink::write(Buffer::Instance& buffer) { parent_.cluster_info_->stats().upstream_cx_rx_bytes_buffered_, parent_.cluster_info_->stats().upstream_cx_tx_bytes_total_, parent_.cluster_info_->stats().upstream_cx_tx_bytes_buffered_, - &parent_.cluster_info_->stats().bind_errors_}); + &parent_.cluster_info_->stats().bind_errors_, nullptr}); connection_->connect(); } diff --git a/test/common/http/conn_manager_impl_test.cc b/test/common/http/conn_manager_impl_test.cc index 266a705a5fb97..398efb334e431 100644 --- a/test/common/http/conn_manager_impl_test.cc +++ b/test/common/http/conn_manager_impl_test.cc @@ -1944,7 +1944,8 @@ TEST_F(HttpConnectionManagerImplTest, DrainClose) { EXPECT_EQ(ssl_connection_.get(), filter->callbacks_->connection()->ssl()); EXPECT_CALL(*codec_, goAway()); - EXPECT_CALL(filter_callbacks_.connection_, close(Network::ConnectionCloseType::FlushWrite)); + EXPECT_CALL(filter_callbacks_.connection_, + close(Network::ConnectionCloseType::FlushWriteAndDelay)); EXPECT_CALL(*drain_timer, disableTimer()); drain_timer->callback_(); @@ -1980,7 +1981,8 @@ TEST_F(HttpConnectionManagerImplTest, ResponseBeforeRequestComplete) { EXPECT_STREQ("envoy-server-test", headers.Server()->value().c_str()); })); EXPECT_CALL(*decoder_filters_[0], onDestroy()); - EXPECT_CALL(filter_callbacks_.connection_, close(Network::ConnectionCloseType::FlushWrite)); + EXPECT_CALL(filter_callbacks_.connection_, + close(Network::ConnectionCloseType::FlushWriteAndDelay)); HeaderMapPtr response_headers{new TestHeaderMapImpl{{":status", "200"}}}; decoder_filters_[0]->callbacks_->encodeHeaders(std::move(response_headers), true); @@ -2033,7 +2035,8 @@ TEST_F(HttpConnectionManagerImplTest, ResponseStartBeforeRequestComplete) { // Since we started the response before the request was complete, we will still close the // connection since we already sent a connection: close header. We won't "reset" the stream // however. - EXPECT_CALL(filter_callbacks_.connection_, close(Network::ConnectionCloseType::FlushWrite)); + EXPECT_CALL(filter_callbacks_.connection_, + close(Network::ConnectionCloseType::FlushWriteAndDelay)); Buffer::OwnedImpl fake_response("world"); filter->callbacks_->encodeData(fake_response, true); } @@ -2069,8 +2072,11 @@ TEST_F(HttpConnectionManagerImplTest, DownstreamProtocolError) { EXPECT_CALL(filter_factory_, createFilterChain(_)).Times(0); - // A protocol exception should result in reset of the streams followed by a local close. - EXPECT_CALL(filter_callbacks_.connection_, close(Network::ConnectionCloseType::FlushWrite)); + // A protocol exception should result in reset of the streams followed by a remote or local close + // depending on whether the downstream client closes the connection prior to the delayed close + // timer firing. + EXPECT_CALL(filter_callbacks_.connection_, + close(Network::ConnectionCloseType::FlushWriteAndDelay)); // Kick off the incoming data. Buffer::OwnedImpl fake_input("1234"); @@ -2136,7 +2142,8 @@ TEST_F(HttpConnectionManagerImplTest, IdleTimeout) { idle_timer->callback_(); EXPECT_CALL(*codec_, goAway()); - EXPECT_CALL(filter_callbacks_.connection_, close(Network::ConnectionCloseType::FlushWrite)); + EXPECT_CALL(filter_callbacks_.connection_, + close(Network::ConnectionCloseType::FlushWriteAndDelay)); EXPECT_CALL(*idle_timer, disableTimer()); EXPECT_CALL(*drain_timer, disableTimer()); drain_timer->callback_(); diff --git a/test/common/network/connection_impl_test.cc b/test/common/network/connection_impl_test.cc index eef797fee4906..8d58c646b45c5 100644 --- a/test/common/network/connection_impl_test.cc +++ b/test/common/network/connection_impl_test.cc @@ -33,6 +33,7 @@ using testing::InSequence; using testing::Invoke; using testing::InvokeWithoutArgs; using testing::Return; +using testing::ReturnNew; using testing::SaveArg; using testing::Sequence; using testing::StrictMock; @@ -358,7 +359,8 @@ TEST_P(ConnectionImplTest, SocketOptionsFailureTest) { struct MockConnectionStats { Connection::ConnectionStats toBufferStats() { - return {rx_total_, rx_current_, tx_total_, tx_current_, &bind_errors_}; + return {rx_total_, rx_current_, tx_total_, + tx_current_, &bind_errors_, &delayed_close_timeouts_}; } StrictMock rx_total_; @@ -366,6 +368,21 @@ struct MockConnectionStats { StrictMock tx_total_; StrictMock tx_current_; StrictMock bind_errors_; + StrictMock delayed_close_timeouts_; +}; + +struct NiceMockConnectionStats { + Connection::ConnectionStats toBufferStats() { + return {rx_total_, rx_current_, tx_total_, + tx_current_, &bind_errors_, &delayed_close_timeouts_}; + } + + NiceMock rx_total_; + NiceMock rx_current_; + NiceMock tx_total_; + NiceMock tx_current_; + NiceMock bind_errors_; + NiceMock delayed_close_timeouts_; }; TEST_P(ConnectionImplTest, ConnectionStats) { @@ -870,6 +887,169 @@ TEST_P(ConnectionImplTest, EmptyReadOnCloseTest) { disconnect(true); } +// Test that a FlushWrite close immediately triggers a close after the write buffer is flushed. +TEST_P(ConnectionImplTest, FlushWriteCloseTest) { + setUpBasicConnection(); + connect(); + server_connection_->setDelayedCloseTimeout(std::chrono::milliseconds(500)); + + std::shared_ptr client_read_filter(new NiceMock()); + client_connection_->addReadFilter(client_read_filter); + + NiceMockConnectionStats stats; + server_connection_->setConnectionStats(stats.toBufferStats()); + + Buffer::OwnedImpl data("data"); + server_connection_->write(data, false); + + // Server connection flushes the write and immediately closes the socket. + // There shouldn't be a read/close race here (see issue #2929), since the client is blocked on + // reading and the connection should close gracefully via FIN. + EXPECT_CALL(*client_read_filter, onData(BufferStringEqual("data"), false)) + .Times(1) + .WillOnce(InvokeWithoutArgs([&]() -> FilterStatus { + dispatcher_->exit(); + return FilterStatus::StopIteration; + })); + EXPECT_CALL(stats.delayed_close_timeouts_, inc()).Times(0); + EXPECT_CALL(client_callbacks_, onEvent(ConnectionEvent::RemoteClose)).Times(1); + EXPECT_CALL(server_callbacks_, onEvent(ConnectionEvent::LocalClose)).Times(1); + server_connection_->close(ConnectionCloseType::FlushWrite); + dispatcher_->run(Event::Dispatcher::RunType::Block); +} + +// Test that a FlushWrite close will trigger a timeout which closes the connection when the write +// buffer is not flushed within the configured interval. +TEST_P(ConnectionImplTest, FlushWriteCloseTimeoutTest) { + NiceMock dispatcher; + EXPECT_CALL(dispatcher.buffer_factory_, create_(_, _)) + .WillRepeatedly(Invoke([](std::function below_low, + std::function above_high) -> Buffer::Instance* { + return new Buffer::WatermarkBuffer(below_low, above_high); + })); + + auto timer = new Event::MockTimer(&dispatcher); + EXPECT_CALL(*timer, enableTimer(_)).Times(1); + EXPECT_CALL(*timer, disableTimer()).Times(1); + + auto file_event = new NiceMock; + EXPECT_CALL(dispatcher, createFileEvent_(0, _, _, _)).WillOnce(Return(file_event)); + + auto transport_socket = new NiceMock; + EXPECT_CALL(*transport_socket, canFlushClose()).WillOnce(Return(true)); + + std::unique_ptr server_connection(new Network::ConnectionImpl( + dispatcher, std::make_unique(0, nullptr, nullptr), + TransportSocketPtr(transport_socket), true)); + server_connection->setDelayedCloseTimeout(std::chrono::milliseconds(500)); + + NiceMockConnectionStats stats; + server_connection->setConnectionStats(stats.toBufferStats()); + EXPECT_CALL(stats.delayed_close_timeouts_, inc()).Times(1); + + Buffer::OwnedImpl data("data"); + server_connection->write(data, false); + // Data is pending in the write buffer, which will trigger the FlushWrite close to go into delayed + // close processing. + server_connection->close(ConnectionCloseType::FlushWrite); + + // Issue the delayed close callback to ensure connection is closed. + timer->callback_(); +} + +// Test that a FlushWriteAndDelay close causes Envoy to flush the write and wait for the client/peer +// to close (until a configured timeout which is not expected to trigger in this test). +TEST_P(ConnectionImplTest, FlushWriteAndDelayCloseTest) { + setUpBasicConnection(); + connect(); + server_connection_->setDelayedCloseTimeout(std::chrono::milliseconds(500)); + + std::shared_ptr client_read_filter(new NiceMock()); + client_connection_->addReadFilter(client_read_filter); + + NiceMockConnectionStats stats; + server_connection_->setConnectionStats(stats.toBufferStats()); + + Buffer::OwnedImpl data("Connection: Close"); + server_connection_->write(data, false); + + EXPECT_CALL(*client_read_filter, onData(BufferStringEqual("Connection: Close"), false)) + .Times(1) + .WillOnce(InvokeWithoutArgs([&]() -> FilterStatus { + client_connection_->close(ConnectionCloseType::NoFlush); + return FilterStatus::StopIteration; + })); + + // Client closes the connection so delayed close timer on the server conn should not fire. + EXPECT_CALL(stats.delayed_close_timeouts_, inc()).Times(0); + EXPECT_CALL(client_callbacks_, onEvent(ConnectionEvent::LocalClose)).Times(1); + EXPECT_CALL(server_callbacks_, onEvent(ConnectionEvent::RemoteClose)) + .Times(1) + .WillOnce(Invoke([&](Network::ConnectionEvent) -> void { dispatcher_->exit(); })); + server_connection_->close(ConnectionCloseType::FlushWriteAndDelay); + dispatcher_->run(Event::Dispatcher::RunType::Block); +} + +// Test that a FlushWriteAndDelay close triggers a timeout which forces Envoy to close the +// connection when a client has not issued a close within the configured interval. +TEST_P(ConnectionImplTest, FlushWriteAndDelayCloseTimerTriggerTest) { + setUpBasicConnection(); + connect(); + server_connection_->setDelayedCloseTimeout(std::chrono::milliseconds(500)); + + std::shared_ptr client_read_filter(new NiceMock()); + client_connection_->addReadFilter(client_read_filter); + + NiceMockConnectionStats stats; + server_connection_->setConnectionStats(stats.toBufferStats()); + + Buffer::OwnedImpl data("Connection: Close"); + server_connection_->write(data, false); + + // The client _will not_ close the connection. Instead, expect the delayed close timer to trigger + // on the server connection. + EXPECT_CALL(*client_read_filter, onData(BufferStringEqual("Connection: Close"), false)) + .Times(1) + .WillOnce(InvokeWithoutArgs([&]() -> FilterStatus { return FilterStatus::StopIteration; })); + EXPECT_CALL(stats.delayed_close_timeouts_, inc()).Times(1); + EXPECT_CALL(client_callbacks_, onEvent(ConnectionEvent::RemoteClose)) + .Times(1) + .WillOnce(Invoke([&](Network::ConnectionEvent) -> void { dispatcher_->exit(); })); + EXPECT_CALL(server_callbacks_, onEvent(ConnectionEvent::LocalClose)).Times(1); + server_connection_->close(ConnectionCloseType::FlushWriteAndDelay); + dispatcher_->run(Event::Dispatcher::RunType::Block); +} + +// Test that delayed close processing can be disabled by setting the delayed close timeout interval +// to 0. +TEST_P(ConnectionImplTest, FlushWriteAndDelayConfigDisabledTest) { + NiceMock callbacks; + NiceMock dispatcher; + EXPECT_CALL(dispatcher.buffer_factory_, create_(_, _)) + .WillRepeatedly(Invoke([](std::function below_low, + std::function above_high) -> Buffer::Instance* { + return new Buffer::WatermarkBuffer(below_low, above_high); + })); + std::unique_ptr server_connection(new Network::ConnectionImpl( + dispatcher, std::make_unique(0, nullptr, nullptr), + std::make_unique>(), true)); + + // Ensure the delayed close timer is not created when the delayedCloseTimeout config value is set + // to 0. + server_connection->setDelayedCloseTimeout(std::chrono::milliseconds(0)); + EXPECT_CALL(dispatcher, createTimer_(_)).Times(0); + + NiceMockConnectionStats stats; + server_connection->setConnectionStats(stats.toBufferStats()); + + EXPECT_CALL(stats.delayed_close_timeouts_, inc()).Times(0); + server_connection->close(ConnectionCloseType::FlushWriteAndDelay); + + // Since the delayed close timer never triggers, the connection never closes. Close it here to end + // the test cleanly due to the (fd == -1) assert in ~ConnectionImpl(). + server_connection->close(ConnectionCloseType::NoFlush); +} + class MockTransportConnectionImplTest : public testing::Test { public: MockTransportConnectionImplTest() { @@ -1251,8 +1431,8 @@ class ReadBufferLimitTest : public ConnectionImplTest { Buffer::OwnedImpl data(std::string(buffer_size, 'a')); client_connection_->write(data, false); dispatcher_->run(Event::Dispatcher::RunType::Block); - } -}; + } // namespace Network +}; // namespace Envoy INSTANTIATE_TEST_CASE_P(IpVersions, ReadBufferLimitTest, testing::ValuesIn(TestEnvironment::getIpVersionsForTest()), diff --git a/test/integration/http_integration.cc b/test/integration/http_integration.cc index 36bf5c9ad2ccb..20b62d7a979c3 100644 --- a/test/integration/http_integration.cc +++ b/test/integration/http_integration.cc @@ -149,18 +149,36 @@ IntegrationCodecClient::startRequest(const Http::HeaderMap& headers) { return {encoder, std::move(response)}; } -void IntegrationCodecClient::waitForDisconnect() { +bool IntegrationCodecClient::waitForDisconnect(uint32_t time_to_wait) { + Event::TimerPtr wait_timer; + bool wait_timer_triggered = false; + if (time_to_wait) { + wait_timer = connection_->dispatcher().createTimer([this, &wait_timer_triggered] { + connection_->dispatcher().exit(); + wait_timer_triggered = true; + }); + wait_timer->enableTimer(std::chrono::milliseconds(time_to_wait)); + } + connection_->dispatcher().run(Event::Dispatcher::RunType::Block); + if (wait_timer_triggered && !disconnected_) { + return false; + } EXPECT_TRUE(disconnected_); + + return true; } void IntegrationCodecClient::ConnectionCallbacks::onEvent(Network::ConnectionEvent event) { + parent_.last_connection_event_ = event; if (event == Network::ConnectionEvent::Connected) { parent_.connected_ = true; parent_.connection_->dispatcher().exit(); } else if (event == Network::ConnectionEvent::RemoteClose) { parent_.disconnected_ = true; parent_.connection_->dispatcher().exit(); + } else { + parent_.disconnected_ = true; } } diff --git a/test/integration/http_integration.h b/test/integration/http_integration.h index b3f342ba50719..d26307b188f3d 100644 --- a/test/integration/http_integration.h +++ b/test/integration/http_integration.h @@ -34,8 +34,9 @@ class IntegrationCodecClient : public Http::CodecClientProd { void sendReset(Http::StreamEncoder& encoder); std::pair startRequest(const Http::HeaderMap& headers); - void waitForDisconnect(); + bool waitForDisconnect(uint32_t time_to_wait = 0); Network::ClientConnection* connection() const { return connection_.get(); } + Network::ConnectionEvent last_connection_event() const { return last_connection_event_; } private: struct ConnectionCallbacks : public Network::ConnectionCallbacks { @@ -66,6 +67,7 @@ class IntegrationCodecClient : public Http::CodecClientProd { bool connected_{}; bool disconnected_{}; bool saw_goaway_{}; + Network::ConnectionEvent last_connection_event_; }; typedef std::unique_ptr IntegrationCodecClientPtr; diff --git a/test/integration/integration_test.cc b/test/integration/integration_test.cc index b7bace2debe10..e4e915b457f28 100644 --- a/test/integration/integration_test.cc +++ b/test/integration/integration_test.cc @@ -364,4 +364,111 @@ TEST_P(IntegrationTest, ViaAppendWith100Continue) { config_helper_.addConfigModifier(setVia("foo")); } +// Test delayed close semantics for downstream HTTP/1.1 connections. When an early response is +// sent by Envoy, it will wait for response acknowledgment (via FIN/RST) from the client before +// closing the socket (with a timeout for ensuring cleanup). +TEST_P(IntegrationTest, TestDelayedConnectionTeardownOnGracefulClose) { + // This test will trigger an early 413 Payload Too Large response due to buffer limits being + // exceeded. The following filter is needed since the router filter will never trigger a 413. + config_helper_.addFilter("{ name: envoy.http_dynamo_filter, config: {} }"); + config_helper_.setBufferLimits(1024, 1024); + initialize(); + + fake_upstreams_[0]->set_allow_unexpected_disconnects(true); + + codec_client_ = makeHttpConnection(lookupPort("http")); + + auto encoder_decoder = + codec_client_->startRequest(Http::TestHeaderMapImpl{{":method", "POST"}, + {":path", "/test/long/url"}, + {":scheme", "http"}, + {":authority", "host"}}); + request_encoder_ = &encoder_decoder.first; + auto response = std::move(encoder_decoder.second); + + codec_client_->sendData(*request_encoder_, 1024 * 65, false); + + response->waitForEndStream(); + EXPECT_TRUE(response->complete()); + EXPECT_STREQ("413", response->headers().Status()->value().c_str()); + // With no delayed close processing, Envoy will close the connection immediately after flushing + // and this should instead return true. + EXPECT_FALSE(codec_client_->waitForDisconnect(500)); + + // Issue a local close and check that the client did not pick up a remote close which can happen + // when delayed close semantics are disabled. + codec_client_->connection()->close(Network::ConnectionCloseType::NoFlush); + EXPECT_EQ(codec_client_->last_connection_event(), Network::ConnectionEvent::LocalClose); +} + +// Test configuration of the delayed close timeout on downstream HTTP/1.1 connections. A value of 0 +// disables delayed close processing. +TEST_P(IntegrationTest, TestDelayedConnectionTeardownConfig) { + config_helper_.addFilter("{ name: envoy.http_dynamo_filter, config: {} }"); + config_helper_.setBufferLimits(1024, 1024); + config_helper_.addConfigModifier( + [](envoy::config::filter::network::http_connection_manager::v2::HttpConnectionManager& hcm) { + hcm.mutable_delayed_close_timeout()->set_seconds(0); + }); + initialize(); + + fake_upstreams_[0]->set_allow_unexpected_disconnects(true); + + codec_client_ = makeHttpConnection(lookupPort("http")); + + auto encoder_decoder = + codec_client_->startRequest(Http::TestHeaderMapImpl{{":method", "POST"}, + {":path", "/test/long/url"}, + {":scheme", "http"}, + {":authority", "host"}}); + request_encoder_ = &encoder_decoder.first; + auto response = std::move(encoder_decoder.second); + + codec_client_->sendData(*request_encoder_, 1024 * 65, false); + + response->waitForEndStream(); + // There is a potential race in the client's response processing when delayed close logic is + // disabled in Envoy (see https://github.com/envoyproxy/envoy/issues/2929). Depending on timing, + // a client may receive an RST prior to reading the response data from the socket, which may clear + // the receive buffers. Also, clients which don't flush the receive buffer upon receiving a remote + // close may also lose data (Envoy is susceptible to this). + // Therefore, avoid checking response code/payload here and instead simply look for the remote + // close. + EXPECT_TRUE(codec_client_->waitForDisconnect(500)); + EXPECT_EQ(codec_client_->last_connection_event(), Network::ConnectionEvent::RemoteClose); +} + +// Test that delay closed connections are eventually force closed when the timeout triggers. +TEST_P(IntegrationTest, TestDelayedConnectionTeardownTimeoutTrigger) { + config_helper_.addFilter("{ name: envoy.http_dynamo_filter, config: {} }"); + config_helper_.setBufferLimits(1024, 1024); + config_helper_.addConfigModifier( + [](envoy::config::filter::network::http_connection_manager::v2::HttpConnectionManager& hcm) { + hcm.mutable_delayed_close_timeout()->set_nanos(500000000); + }); + + initialize(); + + fake_upstreams_[0]->set_allow_unexpected_disconnects(true); + + codec_client_ = makeHttpConnection(lookupPort("http")); + + auto encoder_decoder = + codec_client_->startRequest(Http::TestHeaderMapImpl{{":method", "POST"}, + {":path", "/test/long/url"}, + {":scheme", "http"}, + {":authority", "host"}}); + request_encoder_ = &encoder_decoder.first; + auto response = std::move(encoder_decoder.second); + + codec_client_->sendData(*request_encoder_, 1024 * 65, false); + + response->waitForEndStream(); + // The delayed close timeout should trigger since client is not closing the connection. + EXPECT_TRUE(codec_client_->waitForDisconnect(750)); + EXPECT_EQ(codec_client_->last_connection_event(), Network::ConnectionEvent::RemoteClose); + EXPECT_EQ(test_server_->counter("http.config_test.downstream_cx_delayed_close_timeout")->value(), + 1); +} + } // namespace Envoy diff --git a/test/mocks/network/mocks.h b/test/mocks/network/mocks.h index 318ed47d080c7..9b102234258d6 100644 --- a/test/mocks/network/mocks.h +++ b/test/mocks/network/mocks.h @@ -86,6 +86,8 @@ class MockConnection : public Connection, public MockConnectionBase { MOCK_CONST_METHOD0(localAddressRestored, bool()); MOCK_CONST_METHOD0(aboveHighWatermark, bool()); MOCK_CONST_METHOD0(socketOptions, const Network::ConnectionSocket::OptionsSharedPtr&()); + MOCK_METHOD1(setDelayedCloseTimeout, void(std::chrono::milliseconds)); + MOCK_CONST_METHOD0(delayedCloseTimeout, std::chrono::milliseconds()); }; /** @@ -125,7 +127,8 @@ class MockClientConnection : public ClientConnection, public MockConnectionBase MOCK_CONST_METHOD0(localAddressRestored, bool()); MOCK_CONST_METHOD0(aboveHighWatermark, bool()); MOCK_CONST_METHOD0(socketOptions, const Network::ConnectionSocket::OptionsSharedPtr&()); - + MOCK_METHOD1(setDelayedCloseTimeout, void(std::chrono::milliseconds)); + MOCK_CONST_METHOD0(delayedCloseTimeout, std::chrono::milliseconds()); // Network::ClientConnection MOCK_METHOD0(connect, void()); }; From 94a835a95b66adf73d58f95ead93b84e1b3a15b1 Mon Sep 17 00:00:00 2001 From: Andres Guedez Date: Thu, 6 Sep 2018 16:34:10 -0400 Subject: [PATCH 02/19] Add HTTP/2 integration tests. HTTP/2 integration tests added for delayed close processing. Signed-off-by: Andres Guedez --- test/integration/http2_integration_test.cc | 57 ++++++++++++++++++++++ test/integration/utility.h | 9 +++- 2 files changed, 65 insertions(+), 1 deletion(-) diff --git a/test/integration/http2_integration_test.cc b/test/integration/http2_integration_test.cc index 461db4c97bcb1..c47b66cf27548 100644 --- a/test/integration/http2_integration_test.cc +++ b/test/integration/http2_integration_test.cc @@ -384,6 +384,63 @@ TEST_P(Http2IntegrationTest, SimultaneousRequestWithBufferLimits) { simultaneousRequest(1024 * 32, 1024 * 16); } +// Test downstream connection delayed close processing. +TEST_P(Http2IntegrationTest, DelayedCloseAfterBadFrame) { + initialize(); + Buffer::OwnedImpl buffer("PRI * HTTP/2.0\r\n\r\nSM\r\n\r\nhelloworldcauseanerror"); + std::string response; + RawConnectionDriver connection( + lookupPort("http"), buffer, + [&](Network::ClientConnection& connection, const Buffer::Instance& data) -> void { + response.append(data.toString()); + connection.dispatcher().exit(); + }, + version_); + + connection.run(); + EXPECT_TRUE(response.find("SETTINGS expected") != std::string::npos); + // Due to the multiple dispatchers involved (one for the RawConnectionDriver and another for the + // Envoy server), it's possible the delayed close timer could fire and close the server socket + // prior to the data callback above firing. Therefore, we may either still be connected, or have + // received a remote close. + if (connection.last_connection_event() == Network::ConnectionEvent::Connected) { + connection.run(); + } + EXPECT_EQ(connection.last_connection_event(), Network::ConnectionEvent::RemoteClose); + EXPECT_EQ(test_server_->counter("http.config_test.downstream_cx_delayed_close_timeout")->value(), + 1); +} + +// Test disablement of delayed close processing on downstream connections. +TEST_P(Http2IntegrationTest, DelayedCloseDisabled) { + config_helper_.addConfigModifier( + [](envoy::config::filter::network::http_connection_manager::v2::HttpConnectionManager& hcm) { + hcm.mutable_delayed_close_timeout()->set_seconds(0); + }); + initialize(); + Buffer::OwnedImpl buffer("PRI * HTTP/2.0\r\n\r\nSM\r\n\r\nhelloworldcauseanerror"); + std::string response; + RawConnectionDriver connection( + lookupPort("http"), buffer, + [&](Network::ClientConnection& connection, const Buffer::Instance& data) -> void { + response.append(data.toString()); + connection.dispatcher().exit(); + }, + version_); + + connection.run(); + EXPECT_TRUE(response.find("SETTINGS expected") != std::string::npos); + // Due to the multiple dispatchers involved (one for the RawConnectionDriver and another for the + // Envoy server), it's possible for the 'connection' to receive the data and exit the dispatcher + // prior to the FIN being received from the server. + if (connection.last_connection_event() == Network::ConnectionEvent::Connected) { + connection.run(); + } + EXPECT_EQ(connection.last_connection_event(), Network::ConnectionEvent::RemoteClose); + EXPECT_EQ(test_server_->counter("http.config_test.downstream_cx_delayed_close_timeout")->value(), + 0); +} + Http2RingHashIntegrationTest::Http2RingHashIntegrationTest() { config_helper_.addConfigModifier([&](envoy::config::bootstrap::v2::Bootstrap& bootstrap) -> void { auto* cluster = bootstrap.mutable_static_resources()->mutable_clusters(0); diff --git a/test/integration/utility.h b/test/integration/utility.h index 04e5d650eebb3..adc2903116414 100644 --- a/test/integration/utility.h +++ b/test/integration/utility.h @@ -65,6 +65,9 @@ class RawConnectionDriver { bool connecting() { return callbacks_->connecting_; } void run(Event::Dispatcher::RunType run_type = Event::Dispatcher::RunType::Block); void close(); + Network::ConnectionEvent last_connection_event() const { + return callbacks_->last_connection_event_; + } private: struct ForwardingFilter : public Network::ReadFilterBaseImpl { @@ -83,11 +86,15 @@ class RawConnectionDriver { }; struct ConnectionCallbacks : public Network::ConnectionCallbacks { - void onEvent(Network::ConnectionEvent) override { connecting_ = false; } + void onEvent(Network::ConnectionEvent event) override { + last_connection_event_ = event; + connecting_ = false; + } void onAboveWriteBufferHighWatermark() override {} void onBelowWriteBufferLowWatermark() override {} bool connecting_{true}; + Network::ConnectionEvent last_connection_event_; }; Api::ApiPtr api_; From e2176aa69a6dac7830e5483028ae9044a78fe2eb Mon Sep 17 00:00:00 2001 From: Andres Guedez Date: Mon, 10 Sep 2018 10:57:06 -0400 Subject: [PATCH 03/19] Kick CI Signed-off-by: Andres Guedez From 3a67ecb260465c9899ad43ad36adb7d94f5a7c3d Mon Sep 17 00:00:00 2001 From: Andres Guedez Date: Mon, 10 Sep 2018 13:31:14 -0400 Subject: [PATCH 04/19] Adjust timeout values to prevent test flaking. Increase the timeout values for the common case (no timeout trigerred) tests to prevent test flakiness. Signed-off-by: Andres Guedez --- test/common/network/connection_impl_test.cc | 8 ++++++-- 1 file changed, 6 insertions(+), 2 deletions(-) diff --git a/test/common/network/connection_impl_test.cc b/test/common/network/connection_impl_test.cc index 8d58c646b45c5..a7a191e21a91e 100644 --- a/test/common/network/connection_impl_test.cc +++ b/test/common/network/connection_impl_test.cc @@ -891,7 +891,9 @@ TEST_P(ConnectionImplTest, EmptyReadOnCloseTest) { TEST_P(ConnectionImplTest, FlushWriteCloseTest) { setUpBasicConnection(); connect(); - server_connection_->setDelayedCloseTimeout(std::chrono::milliseconds(500)); + // Set a very high timeout value to prevent flaking. We are testing the common case where the + // timeout does not trigger. + server_connection_->setDelayedCloseTimeout(std::chrono::milliseconds(50000)); std::shared_ptr client_read_filter(new NiceMock()); client_connection_->addReadFilter(client_read_filter); @@ -962,7 +964,9 @@ TEST_P(ConnectionImplTest, FlushWriteCloseTimeoutTest) { TEST_P(ConnectionImplTest, FlushWriteAndDelayCloseTest) { setUpBasicConnection(); connect(); - server_connection_->setDelayedCloseTimeout(std::chrono::milliseconds(500)); + // Set a very high timeout value to prevent flaking. We are testing the common case where the + // timeout does not trigger. + server_connection_->setDelayedCloseTimeout(std::chrono::milliseconds(50000)); std::shared_ptr client_read_filter(new NiceMock()); client_connection_->addReadFilter(client_read_filter); From 30eb02eb23d90e092f05946fccbce4ca60db7487 Mon Sep 17 00:00:00 2001 From: Andres Guedez Date: Tue, 11 Sep 2018 10:07:59 -0400 Subject: [PATCH 05/19] Small cleanup and clarify comments. Signed-off-by: Andres Guedez --- .../v2/http_connection_manager.proto | 4 +++- include/envoy/network/connection.h | 3 ++- source/common/network/connection_impl.cc | 6 +++--- test/common/network/connection_impl_test.cc | 7 ++++++- test/integration/http2_integration_test.cc | 6 ++++-- 5 files changed, 18 insertions(+), 8 deletions(-) diff --git a/api/envoy/config/filter/network/http_connection_manager/v2/http_connection_manager.proto b/api/envoy/config/filter/network/http_connection_manager/v2/http_connection_manager.proto index 768d0999c333e..b4f614562de0a 100644 --- a/api/envoy/config/filter/network/http_connection_manager/v2/http_connection_manager.proto +++ b/api/envoy/config/filter/network/http_connection_manager/v2/http_connection_manager.proto @@ -178,10 +178,12 @@ message HttpConnectionManager { // The delayed close timeout is for downstream connections managed by the HTTP connection manager. // It is defined as a grace period after connection close processing has been locally initiated // during which Envoy will flush the write buffers for the connection and await the peer to close - // the connection (i.e., issue a TCP FIN/RST). + // the connection (i.e., a TCP FIN/RST is received by Envoy from the downstream connection). // // If the timeout triggers, Envoy will close the connection's socket. // + // The default timeout is 1000 ms if this option is not specified. + // // A value of 0 will completely disable delayed close processing, and the downstream connection's // socket will be closed immediately after the write flush is completed. google.protobuf.Duration delayed_close_timeout = 25 [(gogoproto.stdduration) = true]; diff --git a/include/envoy/network/connection.h b/include/envoy/network/connection.h index 9b805546f34d4..06894962d51d4 100644 --- a/include/envoy/network/connection.h +++ b/include/envoy/network/connection.h @@ -64,7 +64,8 @@ class ConnectionCallbacks { enum class ConnectionCloseType { FlushWrite, // Flush pending write data before raising ConnectionEvent::LocalClose NoFlush, // Do not flush any pending data and immediately raise ConnectionEvent::LocalClose - FlushWriteAndDelay + FlushWriteAndDelay // Flush pending write data and delay raising a ConnectionEvent::LocalClose + // until the delayed_close_timeout expires }; /** diff --git a/source/common/network/connection_impl.cc b/source/common/network/connection_impl.cc index 6a7c9fffd094f..1e3cb2bbdd9c6 100644 --- a/source/common/network/connection_impl.cc +++ b/source/common/network/connection_impl.cc @@ -116,7 +116,7 @@ void ConnectionImpl::close(ConnectionCloseType type) { type == ConnectionCloseType::FlushWriteAndDelay, ""); delayed_close_ = true; - bool delayed_close_timeout_set = delayedCloseTimeout().count(); + bool delayed_close_timeout_set = delayedCloseTimeout().count() > 0; // All close types that follow do not actually close() the socket immediately so that buffered // data can be written. However, we do want to stop reading to apply TCP backpressure. @@ -124,13 +124,13 @@ void ConnectionImpl::close(ConnectionCloseType type) { // Force a closeSocket() after the write buffer is flushed if the close_type calls for it or if // no delayed close timeout is set. - close_after_flush_ = type == ConnectionCloseType::FlushWrite || !delayed_close_timeout_set; + close_after_flush_ = !delayed_close_timeout_set || type == ConnectionCloseType::FlushWrite; // Create and activate a timer which will immediately close the connection if triggered. // A config value of 0 disables the timeout. if (delayed_close_timeout_set) { delayed_close_timer_ = dispatcher_.createTimer([this]() -> void { onDelayedCloseTimeout(); }); - ENVOY_CONN_LOG(debug, "setting delayed close timer with timeout {}", *this, + ENVOY_CONN_LOG(debug, "setting delayed close timer with timeout {} ms", *this, delayedCloseTimeout().count()); delayed_close_timer_->enableTimer(delayedCloseTimeout()); } diff --git a/test/common/network/connection_impl_test.cc b/test/common/network/connection_impl_test.cc index a7a191e21a91e..010ac2f7d6cd9 100644 --- a/test/common/network/connection_impl_test.cc +++ b/test/common/network/connection_impl_test.cc @@ -33,7 +33,6 @@ using testing::InSequence; using testing::Invoke; using testing::InvokeWithoutArgs; using testing::Return; -using testing::ReturnNew; using testing::SaveArg; using testing::Sequence; using testing::StrictMock; @@ -962,6 +961,12 @@ TEST_P(ConnectionImplTest, FlushWriteCloseTimeoutTest) { // Test that a FlushWriteAndDelay close causes Envoy to flush the write and wait for the client/peer // to close (until a configured timeout which is not expected to trigger in this test). TEST_P(ConnectionImplTest, FlushWriteAndDelayCloseTest) { +#ifdef __APPLE__ + // libevent does not provide early close notifications on the currently supported macOS builds, so + // the server connection is never notified of the close. For now, we have chosen to disable tests + // that rely on this behavior on macOS (see https://github.com/envoyproxy/envoy/pull/4299). + return; +#endif setUpBasicConnection(); connect(); // Set a very high timeout value to prevent flaking. We are testing the common case where the diff --git a/test/integration/http2_integration_test.cc b/test/integration/http2_integration_test.cc index c47b66cf27548..b49ca312a8a49 100644 --- a/test/integration/http2_integration_test.cc +++ b/test/integration/http2_integration_test.cc @@ -13,7 +13,9 @@ #include "gtest/gtest.h" +using ::testing::HasSubstr; using ::testing::MatchesRegex; + namespace Envoy { INSTANTIATE_TEST_CASE_P(IpVersions, Http2IntegrationTest, @@ -398,7 +400,7 @@ TEST_P(Http2IntegrationTest, DelayedCloseAfterBadFrame) { version_); connection.run(); - EXPECT_TRUE(response.find("SETTINGS expected") != std::string::npos); + EXPECT_THAT(response, HasSubstr("SETTINGS expected")); // Due to the multiple dispatchers involved (one for the RawConnectionDriver and another for the // Envoy server), it's possible the delayed close timer could fire and close the server socket // prior to the data callback above firing. Therefore, we may either still be connected, or have @@ -429,7 +431,7 @@ TEST_P(Http2IntegrationTest, DelayedCloseDisabled) { version_); connection.run(); - EXPECT_TRUE(response.find("SETTINGS expected") != std::string::npos); + EXPECT_THAT(response, HasSubstr("SETTINGS expected")); // Due to the multiple dispatchers involved (one for the RawConnectionDriver and another for the // Envoy server), it's possible for the 'connection' to receive the data and exit the dispatcher // prior to the FIN being received from the server. From 9ddf9d36ac3777fe4d1d6ddc1581291e3ab9e0cc Mon Sep 17 00:00:00 2001 From: Andres Guedez Date: Tue, 11 Sep 2018 11:02:23 -0400 Subject: [PATCH 06/19] Add clarifying comments to unit tests. Signed-off-by: Andres Guedez --- test/common/network/connection_impl_test.cc | 11 +++++++++-- 1 file changed, 9 insertions(+), 2 deletions(-) diff --git a/test/common/network/connection_impl_test.cc b/test/common/network/connection_impl_test.cc index 010ac2f7d6cd9..857fac3570c7f 100644 --- a/test/common/network/connection_impl_test.cc +++ b/test/common/network/connection_impl_test.cc @@ -929,6 +929,8 @@ TEST_P(ConnectionImplTest, FlushWriteCloseTimeoutTest) { return new Buffer::WatermarkBuffer(below_low, above_high); })); + // This timer will be returned to the ConnectionImpl when createTimer() is called to allocate the + // delayed close timer. auto timer = new Event::MockTimer(&dispatcher); EXPECT_CALL(*timer, enableTimer(_)).Times(1); EXPECT_CALL(*timer, disableTimer()).Times(1); @@ -942,7 +944,10 @@ TEST_P(ConnectionImplTest, FlushWriteCloseTimeoutTest) { std::unique_ptr server_connection(new Network::ConnectionImpl( dispatcher, std::make_unique(0, nullptr, nullptr), TransportSocketPtr(transport_socket), true)); - server_connection->setDelayedCloseTimeout(std::chrono::milliseconds(500)); + + // Enable delayed connection close processing by setting a non-zero timeout value. The actual + // value (> 0) doesn't matter since the callback is triggered below. + server_connection->setDelayedCloseTimeout(std::chrono::milliseconds(100)); NiceMockConnectionStats stats; server_connection->setConnectionStats(stats.toBufferStats()); @@ -1004,7 +1009,9 @@ TEST_P(ConnectionImplTest, FlushWriteAndDelayCloseTest) { TEST_P(ConnectionImplTest, FlushWriteAndDelayCloseTimerTriggerTest) { setUpBasicConnection(); connect(); - server_connection_->setDelayedCloseTimeout(std::chrono::milliseconds(500)); + // This timer should always trigger since the client connection does not issue a close() during + // this test. + server_connection_->setDelayedCloseTimeout(std::chrono::milliseconds(50)); std::shared_ptr client_read_filter(new NiceMock()); client_connection_->addReadFilter(client_read_filter); From 8e716eb2703df5a00ba5d58b966032927a0a5d36 Mon Sep 17 00:00:00 2001 From: Andres Guedez Date: Tue, 11 Sep 2018 12:05:58 -0400 Subject: [PATCH 07/19] Disable bazel remote cache due to CI flaking. Signed-off-by: Andres Guedez --- .circleci/config.yml | 5 ----- 1 file changed, 5 deletions(-) diff --git a/.circleci/config.yml b/.circleci/config.yml index f2045b98bef4a..3dae4114d1098 100644 --- a/.circleci/config.yml +++ b/.circleci/config.yml @@ -10,7 +10,6 @@ jobs: resource_class: xlarge working_directory: /source environment: - BAZEL_REMOTE_CACHE: https://storage.googleapis.com/envoy-circleci-bazel-cache/ steps: - run: rm -rf /home/circleci/project/.git # CircleCI git caching is likely broken - checkout @@ -24,7 +23,6 @@ jobs: resource_class: xlarge working_directory: /source environment: - BAZEL_REMOTE_CACHE: https://storage.googleapis.com/envoy-circleci-bazel-cache/ steps: - run: rm -rf /home/circleci/project/.git # CircleCI git caching is likely broken - run: echo $CIRCLE_SHA1 @@ -38,7 +36,6 @@ jobs: resource_class: xlarge working_directory: /source environment: - BAZEL_REMOTE_CACHE: https://storage.googleapis.com/envoy-circleci-bazel-cache/ steps: - run: rm -rf /home/circleci/project/.git # CircleCI git caching is likely broken - checkout @@ -71,7 +68,6 @@ jobs: ipv6_tests: machine: true environment: - BAZEL_REMOTE_CACHE: https://storage.googleapis.com/envoy-circleci-bazel-cache/ steps: - run: rm -rf /home/circleci/project/.git # CircleCI git caching is likely broken - checkout @@ -145,7 +141,6 @@ jobs: macos: xcode: "9.3.0" environment: - BAZEL_REMOTE_CACHE: https://storage.googleapis.com/envoy-circleci-bazel-cache/ steps: - run: sudo ntpdate -vu time.apple.com - run: rm -rf /home/circleci/project/.git # CircleCI git caching is likely broken From e71f7982c4a0dc23bd266c13cd69a17d5229faf8 Mon Sep 17 00:00:00 2001 From: Andres Guedez Date: Tue, 11 Sep 2018 23:00:12 -0400 Subject: [PATCH 08/19] Minor cleanup per style guidelines and best practices Signed-off-by: Andres Guedez --- source/common/network/connection_impl.cc | 5 ++--- test/common/network/connection_impl_test.cc | 16 +++++++++------- 2 files changed, 11 insertions(+), 10 deletions(-) diff --git a/source/common/network/connection_impl.cc b/source/common/network/connection_impl.cc index 1e3cb2bbdd9c6..521edaadcdf5f 100644 --- a/source/common/network/connection_impl.cc +++ b/source/common/network/connection_impl.cc @@ -112,9 +112,8 @@ void ConnectionImpl::close(ConnectionCloseType type) { closeSocket(ConnectionEvent::LocalClose); } else { - RELEASE_ASSERT(type == ConnectionCloseType::FlushWrite || - type == ConnectionCloseType::FlushWriteAndDelay, - ""); + ASSERT(type == ConnectionCloseType::FlushWrite || + type == ConnectionCloseType::FlushWriteAndDelay); delayed_close_ = true; bool delayed_close_timeout_set = delayedCloseTimeout().count() > 0; diff --git a/test/common/network/connection_impl_test.cc b/test/common/network/connection_impl_test.cc index 857fac3570c7f..ef32ed84c7a70 100644 --- a/test/common/network/connection_impl_test.cc +++ b/test/common/network/connection_impl_test.cc @@ -926,24 +926,26 @@ TEST_P(ConnectionImplTest, FlushWriteCloseTimeoutTest) { EXPECT_CALL(dispatcher.buffer_factory_, create_(_, _)) .WillRepeatedly(Invoke([](std::function below_low, std::function above_high) -> Buffer::Instance* { + // ConnectionImpl calls Envoy::MockBufferFactory::create(), which calls create_() and wraps + // the returned raw pointer below with a unique_ptr. return new Buffer::WatermarkBuffer(below_low, above_high); })); - // This timer will be returned to the ConnectionImpl when createTimer() is called to allocate the - // delayed close timer. + // This timer will be returned (transferring ownership) to the ConnectionImpl when createTimer() + // is called to allocate the delayed close timer. auto timer = new Event::MockTimer(&dispatcher); EXPECT_CALL(*timer, enableTimer(_)).Times(1); EXPECT_CALL(*timer, disableTimer()).Times(1); - auto file_event = new NiceMock; - EXPECT_CALL(dispatcher, createFileEvent_(0, _, _, _)).WillOnce(Return(file_event)); + auto file_event = std::make_unique>(); + EXPECT_CALL(dispatcher, createFileEvent_(0, _, _, _)).WillOnce(Return(file_event.release())); - auto transport_socket = new NiceMock; + auto transport_socket = std::make_unique>(); EXPECT_CALL(*transport_socket, canFlushClose()).WillOnce(Return(true)); - std::unique_ptr server_connection(new Network::ConnectionImpl( + auto server_connection = std::make_unique( dispatcher, std::make_unique(0, nullptr, nullptr), - TransportSocketPtr(transport_socket), true)); + std::move(transport_socket), true); // Enable delayed connection close processing by setting a non-zero timeout value. The actual // value (> 0) doesn't matter since the callback is triggered below. From 6130f2843cdb10f274b719bef25a9ffeb9909db4 Mon Sep 17 00:00:00 2001 From: Andres Guedez Date: Thu, 13 Sep 2018 09:30:23 -0400 Subject: [PATCH 09/19] Cleanup: comments and asserts/checks. Signed-off-by: Andres Guedez --- include/envoy/network/connection.h | 3 +-- source/common/network/connection_impl.cc | 17 +++++++++++++++-- test/integration/http_integration.cc | 12 +++++++++--- test/integration/http_integration.h | 2 +- test/integration/integration_test.cc | 6 +++--- 5 files changed, 29 insertions(+), 11 deletions(-) diff --git a/include/envoy/network/connection.h b/include/envoy/network/connection.h index 06894962d51d4..3926d97d2f947 100644 --- a/include/envoy/network/connection.h +++ b/include/envoy/network/connection.h @@ -88,7 +88,7 @@ class Connection : public Event::DeferredDeletable, public FilterManager { Stats::Gauge& write_current_; // Counter* as this is an optional counter. Bind errors will not be tracked if this is nullptr. Stats::Counter* bind_errors_; - // Optional counter. Delayed close semantics are only used by HTTP connections. + // Optional counter. Delayed close timeouts will not be tracked if this is nullptr. Stats::Counter* delayed_close_timeouts_; }; @@ -240,7 +240,6 @@ class Connection : public Event::DeferredDeletable, public FilterManager { /** * Set the timeout for delayed connection close()s. - * This is only used for downstream connections processing HTTP/1 and HTTP/2. * @param timeout The timeout value in milliseconds */ virtual void setDelayedCloseTimeout(std::chrono::milliseconds timeout) PURE; diff --git a/source/common/network/connection_impl.cc b/source/common/network/connection_impl.cc index 521edaadcdf5f..78fc55b5142f6 100644 --- a/source/common/network/connection_impl.cc +++ b/source/common/network/connection_impl.cc @@ -73,7 +73,6 @@ ConnectionImpl::~ConnectionImpl() { if (delayed_close_timer_) { // It's ok to disable even if the timer has already fired. delayed_close_timer_->disableTimer(); - delayed_close_timer_.reset(); } // In general we assume that owning code has called close() previously to the destructor being @@ -114,9 +113,23 @@ void ConnectionImpl::close(ConnectionCloseType type) { } else { ASSERT(type == ConnectionCloseType::FlushWrite || type == ConnectionCloseType::FlushWriteAndDelay); + + // No need to continue if a FlushWrite/FlushWriteAndDelay has already been issued and there is a + // pending delayed close. + if (delayed_close_) { + return; + } + delayed_close_ = true; bool delayed_close_timeout_set = delayedCloseTimeout().count() > 0; + // NOTE: the delayed close timeout (if set) affects both FlushWrite and FlushWriteAndDelay + // closes: + // 1. For FlushWrite, the timeout sets an upper bound on how long to wait for the flush to + // complete before the connection is locally closed. + // 2. For FlushWriteAndDelay, the timeout specifies an upper bound on how long to wait for the + // flush to complete and the peer to close the connection before it is locally closed. + // All close types that follow do not actually close() the socket immediately so that buffered // data can be written. However, we do want to stop reading to apply TCP backpressure. read_enabled_ = false; @@ -128,6 +141,7 @@ void ConnectionImpl::close(ConnectionCloseType type) { // Create and activate a timer which will immediately close the connection if triggered. // A config value of 0 disables the timeout. if (delayed_close_timeout_set) { + ASSERT(connection_stats_->delayed_close_timeouts_ != nullptr); delayed_close_timer_ = dispatcher_.createTimer([this]() -> void { onDelayedCloseTimeout(); }); ENVOY_CONN_LOG(debug, "setting delayed close timer with timeout {} ms", *this, delayedCloseTimeout().count()); @@ -561,7 +575,6 @@ bool ConnectionImpl::bothSidesHalfClosed() { void ConnectionImpl::onDelayedCloseTimeout() { ENVOY_CONN_LOG(debug, "triggered delayed close", *this); - ASSERT(connection_stats_->delayed_close_timeouts_); connection_stats_->delayed_close_timeouts_->inc(); closeSocket(ConnectionEvent::LocalClose); } diff --git a/test/integration/http_integration.cc b/test/integration/http_integration.cc index 20b62d7a979c3..69f02b5daa107 100644 --- a/test/integration/http_integration.cc +++ b/test/integration/http_integration.cc @@ -149,18 +149,24 @@ IntegrationCodecClient::startRequest(const Http::HeaderMap& headers) { return {encoder, std::move(response)}; } -bool IntegrationCodecClient::waitForDisconnect(uint32_t time_to_wait) { +bool IntegrationCodecClient::waitForDisconnect(std::chrono::milliseconds time_to_wait) { Event::TimerPtr wait_timer; bool wait_timer_triggered = false; - if (time_to_wait) { + if (time_to_wait.count()) { wait_timer = connection_->dispatcher().createTimer([this, &wait_timer_triggered] { connection_->dispatcher().exit(); wait_timer_triggered = true; }); - wait_timer->enableTimer(std::chrono::milliseconds(time_to_wait)); + wait_timer->enableTimer(time_to_wait); } connection_->dispatcher().run(Event::Dispatcher::RunType::Block); + + // Disable the timer if it was created. This call is harmless if the timer already triggered. + if (wait_timer) { + wait_timer->disableTimer(); + } + if (wait_timer_triggered && !disconnected_) { return false; } diff --git a/test/integration/http_integration.h b/test/integration/http_integration.h index d26307b188f3d..4c814020dbb12 100644 --- a/test/integration/http_integration.h +++ b/test/integration/http_integration.h @@ -34,7 +34,7 @@ class IntegrationCodecClient : public Http::CodecClientProd { void sendReset(Http::StreamEncoder& encoder); std::pair startRequest(const Http::HeaderMap& headers); - bool waitForDisconnect(uint32_t time_to_wait = 0); + bool waitForDisconnect(std::chrono::milliseconds time_to_wait = std::chrono::milliseconds(0)); Network::ClientConnection* connection() const { return connection_.get(); } Network::ConnectionEvent last_connection_event() const { return last_connection_event_; } diff --git a/test/integration/integration_test.cc b/test/integration/integration_test.cc index e4e915b457f28..eb5abd89bf3b0 100644 --- a/test/integration/integration_test.cc +++ b/test/integration/integration_test.cc @@ -393,7 +393,7 @@ TEST_P(IntegrationTest, TestDelayedConnectionTeardownOnGracefulClose) { EXPECT_STREQ("413", response->headers().Status()->value().c_str()); // With no delayed close processing, Envoy will close the connection immediately after flushing // and this should instead return true. - EXPECT_FALSE(codec_client_->waitForDisconnect(500)); + EXPECT_FALSE(codec_client_->waitForDisconnect(std::chrono::milliseconds(500))); // Issue a local close and check that the client did not pick up a remote close which can happen // when delayed close semantics are disabled. @@ -434,7 +434,7 @@ TEST_P(IntegrationTest, TestDelayedConnectionTeardownConfig) { // close may also lose data (Envoy is susceptible to this). // Therefore, avoid checking response code/payload here and instead simply look for the remote // close. - EXPECT_TRUE(codec_client_->waitForDisconnect(500)); + EXPECT_TRUE(codec_client_->waitForDisconnect(std::chrono::milliseconds(500))); EXPECT_EQ(codec_client_->last_connection_event(), Network::ConnectionEvent::RemoteClose); } @@ -465,7 +465,7 @@ TEST_P(IntegrationTest, TestDelayedConnectionTeardownTimeoutTrigger) { response->waitForEndStream(); // The delayed close timeout should trigger since client is not closing the connection. - EXPECT_TRUE(codec_client_->waitForDisconnect(750)); + EXPECT_TRUE(codec_client_->waitForDisconnect(std::chrono::milliseconds(750))); EXPECT_EQ(codec_client_->last_connection_event(), Network::ConnectionEvent::RemoteClose); EXPECT_EQ(test_server_->counter("http.config_test.downstream_cx_delayed_close_timeout")->value(), 1); From 890471e6497c53c0cc7b530ff40edc27c64c9924 Mon Sep 17 00:00:00 2001 From: Andres Guedez Date: Thu, 13 Sep 2018 09:53:51 -0400 Subject: [PATCH 10/19] Adjust integration test timeouts for flake resistance. Signed-off-by: Andres Guedez --- test/integration/integration_test.cc | 5 +++-- 1 file changed, 3 insertions(+), 2 deletions(-) diff --git a/test/integration/integration_test.cc b/test/integration/integration_test.cc index eb5abd89bf3b0..4ffc97dedf1ef 100644 --- a/test/integration/integration_test.cc +++ b/test/integration/integration_test.cc @@ -444,7 +444,8 @@ TEST_P(IntegrationTest, TestDelayedConnectionTeardownTimeoutTrigger) { config_helper_.setBufferLimits(1024, 1024); config_helper_.addConfigModifier( [](envoy::config::filter::network::http_connection_manager::v2::HttpConnectionManager& hcm) { - hcm.mutable_delayed_close_timeout()->set_nanos(500000000); + // 200ms. + hcm.mutable_delayed_close_timeout()->set_nanos(200000000); }); initialize(); @@ -465,7 +466,7 @@ TEST_P(IntegrationTest, TestDelayedConnectionTeardownTimeoutTrigger) { response->waitForEndStream(); // The delayed close timeout should trigger since client is not closing the connection. - EXPECT_TRUE(codec_client_->waitForDisconnect(std::chrono::milliseconds(750))); + EXPECT_TRUE(codec_client_->waitForDisconnect(std::chrono::milliseconds(2000))); EXPECT_EQ(codec_client_->last_connection_event(), Network::ConnectionEvent::RemoteClose); EXPECT_EQ(test_server_->counter("http.config_test.downstream_cx_delayed_close_timeout")->value(), 1); From aae3b52558f5ac5063943e6718e0b9015a9b0d77 Mon Sep 17 00:00:00 2001 From: Andres Guedez Date: Thu, 13 Sep 2018 11:04:24 -0400 Subject: [PATCH 11/19] Revert namespace comments erronously introduced. Signed-off-by: Andres Guedez --- test/common/network/connection_impl_test.cc | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/test/common/network/connection_impl_test.cc b/test/common/network/connection_impl_test.cc index ef32ed84c7a70..94bd7fcaa64ad 100644 --- a/test/common/network/connection_impl_test.cc +++ b/test/common/network/connection_impl_test.cc @@ -1449,8 +1449,8 @@ class ReadBufferLimitTest : public ConnectionImplTest { Buffer::OwnedImpl data(std::string(buffer_size, 'a')); client_connection_->write(data, false); dispatcher_->run(Event::Dispatcher::RunType::Block); - } // namespace Network -}; // namespace Envoy + } +}; INSTANTIATE_TEST_CASE_P(IpVersions, ReadBufferLimitTest, testing::ValuesIn(TestEnvironment::getIpVersionsForTest()), From b8eb565e16da69b8e50a41291851cf88d6a0e4d4 Mon Sep 17 00:00:00 2001 From: Andres Guedez Date: Thu, 13 Sep 2018 12:24:49 -0400 Subject: [PATCH 12/19] Kick CI Signed-off-by: Andres Guedez From 6071d31e86370e64a34f593b4d79ecd8ad7b61a3 Mon Sep 17 00:00:00 2001 From: Andres Guedez Date: Thu, 13 Sep 2018 15:49:08 -0400 Subject: [PATCH 13/19] Change ASSERT to runtime check. Signed-off-by: Andres Guedez --- source/common/network/connection_impl.cc | 5 +++-- 1 file changed, 3 insertions(+), 2 deletions(-) diff --git a/source/common/network/connection_impl.cc b/source/common/network/connection_impl.cc index 78fc55b5142f6..ff4c5e5f0cda2 100644 --- a/source/common/network/connection_impl.cc +++ b/source/common/network/connection_impl.cc @@ -141,7 +141,6 @@ void ConnectionImpl::close(ConnectionCloseType type) { // Create and activate a timer which will immediately close the connection if triggered. // A config value of 0 disables the timeout. if (delayed_close_timeout_set) { - ASSERT(connection_stats_->delayed_close_timeouts_ != nullptr); delayed_close_timer_ = dispatcher_.createTimer([this]() -> void { onDelayedCloseTimeout(); }); ENVOY_CONN_LOG(debug, "setting delayed close timer with timeout {} ms", *this, delayedCloseTimeout().count()); @@ -575,7 +574,9 @@ bool ConnectionImpl::bothSidesHalfClosed() { void ConnectionImpl::onDelayedCloseTimeout() { ENVOY_CONN_LOG(debug, "triggered delayed close", *this); - connection_stats_->delayed_close_timeouts_->inc(); + if (connection_stats_->delayed_close_timeouts_ != nullptr) { + connection_stats_->delayed_close_timeouts_->inc(); + } closeSocket(ConnectionEvent::LocalClose); } From 36b608750dda9c961e949bd6074aee172e6c9d7c Mon Sep 17 00:00:00 2001 From: Andres Guedez Date: Sun, 16 Sep 2018 20:54:19 -0400 Subject: [PATCH 14/19] Improve delayed_close_timeout comment. Signed-off-by: Andres Guedez --- .../v2/http_connection_manager.proto | 8 +++++++- 1 file changed, 7 insertions(+), 1 deletion(-) diff --git a/api/envoy/config/filter/network/http_connection_manager/v2/http_connection_manager.proto b/api/envoy/config/filter/network/http_connection_manager/v2/http_connection_manager.proto index b4f614562de0a..ef4aaddbce9ac 100644 --- a/api/envoy/config/filter/network/http_connection_manager/v2/http_connection_manager.proto +++ b/api/envoy/config/filter/network/http_connection_manager/v2/http_connection_manager.proto @@ -178,7 +178,13 @@ message HttpConnectionManager { // The delayed close timeout is for downstream connections managed by the HTTP connection manager. // It is defined as a grace period after connection close processing has been locally initiated // during which Envoy will flush the write buffers for the connection and await the peer to close - // the connection (i.e., a TCP FIN/RST is received by Envoy from the downstream connection). + // (i.e., a TCP FIN/RST is received by Envoy from the downstream connection). + // + // Delaying Envoy's connection close and giving the peer the opportunity to initate the close + // sequence mitigates a race condition that exists when downstream clients do not drain/process + // data in a connection's receive buffer after a remote close has been detected via a socket + // write(). This race leads to such clients failing to process the response code sent by Envoy, + // which could result in erroneous downstream processing. // // If the timeout triggers, Envoy will close the connection's socket. // From d056d7a7f3b4e25f9e0cd0538458d330b19abb9c Mon Sep 17 00:00:00 2001 From: Andres Guedez Date: Sun, 16 Sep 2018 20:55:49 -0400 Subject: [PATCH 15/19] Revert disablement of bazel remote caching. Remote caching issue is being tracked in #4407. Signed-off-by: Andres Guedez --- .circleci/config.yml | 5 +++++ 1 file changed, 5 insertions(+) diff --git a/.circleci/config.yml b/.circleci/config.yml index 3dae4114d1098..f2045b98bef4a 100644 --- a/.circleci/config.yml +++ b/.circleci/config.yml @@ -10,6 +10,7 @@ jobs: resource_class: xlarge working_directory: /source environment: + BAZEL_REMOTE_CACHE: https://storage.googleapis.com/envoy-circleci-bazel-cache/ steps: - run: rm -rf /home/circleci/project/.git # CircleCI git caching is likely broken - checkout @@ -23,6 +24,7 @@ jobs: resource_class: xlarge working_directory: /source environment: + BAZEL_REMOTE_CACHE: https://storage.googleapis.com/envoy-circleci-bazel-cache/ steps: - run: rm -rf /home/circleci/project/.git # CircleCI git caching is likely broken - run: echo $CIRCLE_SHA1 @@ -36,6 +38,7 @@ jobs: resource_class: xlarge working_directory: /source environment: + BAZEL_REMOTE_CACHE: https://storage.googleapis.com/envoy-circleci-bazel-cache/ steps: - run: rm -rf /home/circleci/project/.git # CircleCI git caching is likely broken - checkout @@ -68,6 +71,7 @@ jobs: ipv6_tests: machine: true environment: + BAZEL_REMOTE_CACHE: https://storage.googleapis.com/envoy-circleci-bazel-cache/ steps: - run: rm -rf /home/circleci/project/.git # CircleCI git caching is likely broken - checkout @@ -141,6 +145,7 @@ jobs: macos: xcode: "9.3.0" environment: + BAZEL_REMOTE_CACHE: https://storage.googleapis.com/envoy-circleci-bazel-cache/ steps: - run: sudo ntpdate -vu time.apple.com - run: rm -rf /home/circleci/project/.git # CircleCI git caching is likely broken From 9d86332875a68287332613c0427b4f125ca25ef7 Mon Sep 17 00:00:00 2001 From: Andres Guedez Date: Fri, 21 Sep 2018 16:42:07 -0400 Subject: [PATCH 16/19] Add clarifying comment and minor const correctness fix. Signed-off-by: Andres Guedez --- source/common/network/connection_impl.cc | 11 ++++++++++- 1 file changed, 10 insertions(+), 1 deletion(-) diff --git a/source/common/network/connection_impl.cc b/source/common/network/connection_impl.cc index ff4c5e5f0cda2..d5eed98838ca4 100644 --- a/source/common/network/connection_impl.cc +++ b/source/common/network/connection_impl.cc @@ -116,12 +116,21 @@ void ConnectionImpl::close(ConnectionCloseType type) { // No need to continue if a FlushWrite/FlushWriteAndDelay has already been issued and there is a // pending delayed close. + // + // An example of this condition manifests when a downstream connection is closed early by Envoy, + // such as when a route can't be matched: + // In ConnectionManagerImpl::onData() + // 1) Via codec_->dispatch(), a local reply with a 404 is sent to the client + // a) ConnectionManagerImpl::doEndStream() issues the first connection close() via + // ConnectionManagerImpl::checkForDeferredClose() + // 2) A second close is issued by a subsequent call to + // ConnectionManagerImpl::checkForDeferredClose() prior to returning from onData() if (delayed_close_) { return; } delayed_close_ = true; - bool delayed_close_timeout_set = delayedCloseTimeout().count() > 0; + const bool delayed_close_timeout_set = delayedCloseTimeout().count() > 0; // NOTE: the delayed close timeout (if set) affects both FlushWrite and FlushWriteAndDelay // closes: From 435a518041a38f05d0cb0dbadbd3e50a6670d695 Mon Sep 17 00:00:00 2001 From: Andres Guedez Date: Sat, 22 Sep 2018 21:56:25 -0400 Subject: [PATCH 17/19] Revert changes to http{1,2} codecs. Instead, constrain setting the delayed close timeout to the HCM config, which owns it. Signed-off-by: Andres Guedez --- source/common/http/http1/codec_impl.cc | 7 ++----- source/common/http/http1/codec_impl.h | 5 ++--- source/common/http/http2/codec_impl.cc | 4 +--- source/common/http/http2/codec_impl.h | 6 ++---- .../network/http_connection_manager/config.cc | 13 +++++++------ 5 files changed, 14 insertions(+), 21 deletions(-) diff --git a/source/common/http/http1/codec_impl.cc b/source/common/http/http1/codec_impl.cc index 0dbadaa47c92d..24d9d6d49c3e1 100644 --- a/source/common/http/http1/codec_impl.cc +++ b/source/common/http/http1/codec_impl.cc @@ -450,11 +450,8 @@ void ConnectionImpl::onResetStreamBase(StreamResetReason reason) { ServerConnectionImpl::ServerConnectionImpl(Network::Connection& connection, ServerConnectionCallbacks& callbacks, - Http1Settings settings, - std::chrono::milliseconds delayed_close_timeout) - : ConnectionImpl(connection, HTTP_REQUEST), callbacks_(callbacks), codec_settings_(settings) { - connection_.setDelayedCloseTimeout(delayed_close_timeout); -} + Http1Settings settings) + : ConnectionImpl(connection, HTTP_REQUEST), callbacks_(callbacks), codec_settings_(settings) {} void ServerConnectionImpl::onEncodeComplete() { ASSERT(active_request_); diff --git a/source/common/http/http1/codec_impl.h b/source/common/http/http1/codec_impl.h index e974aa03187e1..6f8cafd2c9714 100644 --- a/source/common/http/http1/codec_impl.h +++ b/source/common/http/http1/codec_impl.h @@ -277,9 +277,8 @@ class ConnectionImpl : public virtual Connection, protected Logger::Loggable Date: Mon, 24 Sep 2018 09:59:27 -0400 Subject: [PATCH 18/19] Set the delayed close timeout for a connection in the HCM. Signed-off-by: Andres Guedez --- source/common/http/conn_manager_config.h | 6 ++++++ source/common/http/conn_manager_impl.cc | 2 ++ .../filters/network/http_connection_manager/config.cc | 1 - .../filters/network/http_connection_manager/config.h | 2 +- source/server/http/admin.h | 1 + test/common/http/conn_manager_impl_fuzz_test.cc | 2 ++ test/common/http/conn_manager_impl_test.cc | 2 ++ test/common/http/conn_manager_utility_test.cc | 1 + 8 files changed, 15 insertions(+), 2 deletions(-) diff --git a/source/common/http/conn_manager_config.h b/source/common/http/conn_manager_config.h index 7d4cd07838ff9..3f9c3345aa919 100644 --- a/source/common/http/conn_manager_config.h +++ b/source/common/http/conn_manager_config.h @@ -204,6 +204,12 @@ class ConnectionManagerConfig { */ virtual std::chrono::milliseconds streamIdleTimeout() const PURE; + /** + * @return delayed close timeout for downstream HTTP connections. Zero indicates a disabled + * timeout. See http_connection_manager.proto for a detailed description of this timeout. + */ + virtual std::chrono::milliseconds delayedCloseTimeout() const PURE; + /** * @return Router::RouteConfigProvider& the configuration provider used to acquire a route * config for each request flow. diff --git a/source/common/http/conn_manager_impl.cc b/source/common/http/conn_manager_impl.cc index 0ac83e0eadcf6..b9fd92289f719 100644 --- a/source/common/http/conn_manager_impl.cc +++ b/source/common/http/conn_manager_impl.cc @@ -87,6 +87,8 @@ void ConnectionManagerImpl::initializeReadFilterCallbacks(Network::ReadFilterCal idle_timer_->enableTimer(config_.idleTimeout().value()); } + read_callbacks_->connection().setDelayedCloseTimeout(config_.delayedCloseTimeout()); + read_callbacks_->connection().setConnectionStats( {stats_.named_.downstream_cx_rx_bytes_total_, stats_.named_.downstream_cx_rx_bytes_buffered_, stats_.named_.downstream_cx_tx_bytes_total_, stats_.named_.downstream_cx_tx_bytes_buffered_, diff --git a/source/extensions/filters/network/http_connection_manager/config.cc b/source/extensions/filters/network/http_connection_manager/config.cc index 47bc209aca431..c91757b58872f 100644 --- a/source/extensions/filters/network/http_connection_manager/config.cc +++ b/source/extensions/filters/network/http_connection_manager/config.cc @@ -302,7 +302,6 @@ Http::ServerConnectionPtr HttpConnectionManagerConfig::createCodec(Network::Connection& connection, const Buffer::Instance& data, Http::ServerConnectionCallbacks& callbacks) { - connection.setDelayedCloseTimeout(delayed_close_timeout_); switch (codec_type_) { case CodecType::HTTP1: return Http::ServerConnectionPtr{ diff --git a/source/extensions/filters/network/http_connection_manager/config.h b/source/extensions/filters/network/http_connection_manager/config.h index b8bf8fb8259f6..99085341b1c8c 100644 --- a/source/extensions/filters/network/http_connection_manager/config.h +++ b/source/extensions/filters/network/http_connection_manager/config.h @@ -109,7 +109,7 @@ class HttpConnectionManagerConfig : Logger::Loggable, Http::ConnectionManagerListenerStats& listenerStats() override { return listener_stats_; } bool proxy100Continue() const override { return proxy_100_continue_; } const Http::Http1Settings& http1Settings() const override { return http1_settings_; } - std::chrono::milliseconds delayedCloseTimeout() const { return delayed_close_timeout_; } + std::chrono::milliseconds delayedCloseTimeout() const override { return delayed_close_timeout_; } private: typedef std::list FilterFactoriesList; diff --git a/source/server/http/admin.h b/source/server/http/admin.h index 22175186cc55d..6e329202ce641 100644 --- a/source/server/http/admin.h +++ b/source/server/http/admin.h @@ -94,6 +94,7 @@ class AdminImpl : public Admin, bool generateRequestId() override { return false; } absl::optional idleTimeout() const override { return idle_timeout_; } std::chrono::milliseconds streamIdleTimeout() const override { return {}; } + std::chrono::milliseconds delayedCloseTimeout() const override { return {}; } Router::RouteConfigProvider& routeConfigProvider() override { return route_config_provider_; } const std::string& serverName() override { return Http::DefaultServerString::get(); } Http::ConnectionManagerStats& stats() override { return stats_; } diff --git a/test/common/http/conn_manager_impl_fuzz_test.cc b/test/common/http/conn_manager_impl_fuzz_test.cc index 27fdf81189613..a7ea8222353be 100644 --- a/test/common/http/conn_manager_impl_fuzz_test.cc +++ b/test/common/http/conn_manager_impl_fuzz_test.cc @@ -90,6 +90,7 @@ class FuzzConfig : public ConnectionManagerConfig { bool generateRequestId() override { return true; } absl::optional idleTimeout() const override { return idle_timeout_; } std::chrono::milliseconds streamIdleTimeout() const override { return stream_idle_timeout_; } + std::chrono::milliseconds delayedCloseTimeout() const override { return delayed_close_timeout_; } Router::RouteConfigProvider& routeConfigProvider() override { return route_config_provider_; } const std::string& serverName() override { return server_name_; } ConnectionManagerStats& stats() override { return stats_; } @@ -125,6 +126,7 @@ class FuzzConfig : public ConnectionManagerConfig { ConnectionManagerTracingStats tracing_stats_; ConnectionManagerListenerStats listener_stats_; std::chrono::milliseconds stream_idle_timeout_{}; + std::chrono::milliseconds delayed_close_timeout_{}; bool use_remote_address_{true}; Http::ForwardClientCertType forward_client_cert_{Http::ForwardClientCertType::Sanitize}; std::vector set_current_client_cert_details_; diff --git a/test/common/http/conn_manager_impl_test.cc b/test/common/http/conn_manager_impl_test.cc index 8123ed6ae6176..349f25b29690e 100644 --- a/test/common/http/conn_manager_impl_test.cc +++ b/test/common/http/conn_manager_impl_test.cc @@ -258,6 +258,7 @@ class HttpConnectionManagerImplTest : public Test, public ConnectionManagerConfi bool generateRequestId() override { return true; } absl::optional idleTimeout() const override { return idle_timeout_; } std::chrono::milliseconds streamIdleTimeout() const override { return stream_idle_timeout_; } + std::chrono::milliseconds delayedCloseTimeout() const override { return delayed_close_timeout_; } Router::RouteConfigProvider& routeConfigProvider() override { return route_config_provider_; } const std::string& serverName() override { return server_name_; } ConnectionManagerStats& stats() override { return stats_; } @@ -300,6 +301,7 @@ class HttpConnectionManagerImplTest : public Test, public ConnectionManagerConfi absl::optional user_agent_; absl::optional idle_timeout_; std::chrono::milliseconds stream_idle_timeout_{}; + std::chrono::milliseconds delayed_close_timeout_{}; NiceMock random_; NiceMock local_info_; NiceMock factory_context_; diff --git a/test/common/http/conn_manager_utility_test.cc b/test/common/http/conn_manager_utility_test.cc index 1e987304400e0..09bd2f3282cd5 100644 --- a/test/common/http/conn_manager_utility_test.cc +++ b/test/common/http/conn_manager_utility_test.cc @@ -46,6 +46,7 @@ class MockConnectionManagerConfig : public ConnectionManagerConfig { MOCK_METHOD0(generateRequestId, bool()); MOCK_CONST_METHOD0(idleTimeout, absl::optional()); MOCK_CONST_METHOD0(streamIdleTimeout, std::chrono::milliseconds()); + MOCK_CONST_METHOD0(delayedCloseTimeout, std::chrono::milliseconds()); MOCK_METHOD0(routeConfigProvider, Router::RouteConfigProvider&()); MOCK_METHOD0(serverName, const std::string&()); MOCK_METHOD0(stats, ConnectionManagerStats&()); From 31bcaaa66d548f6b365c4de13d8a0265736a1c0f Mon Sep 17 00:00:00 2001 From: Andres Guedez Date: Mon, 24 Sep 2018 16:45:05 -0400 Subject: [PATCH 19/19] Update release notes with delayed close timeout. Signed-off-by: Andres Guedez --- docs/root/intro/version_history.rst | 1 + 1 file changed, 1 insertion(+) diff --git a/docs/root/intro/version_history.rst b/docs/root/intro/version_history.rst index 8019f7e6c7f3c..41b5889846ba4 100644 --- a/docs/root/intro/version_history.rst +++ b/docs/root/intro/version_history.rst @@ -57,6 +57,7 @@ Version history dynamic table size of both: encoder and decoder. * http: added support for removing request headers using :ref:`request_headers_to_remove `. +* http: added support for a :ref:`delayed close timeout` to mitigate race conditions when closing connections to downstream HTTP clients. The timeout defaults to 1 second. * jwt-authn filter: add support for per route JWT requirements. * listeners: added the ability to match :ref:`FilterChain ` using :ref:`destination_port ` and