From d73ee1d0cf0bb0c9381509931520af3d286e865c Mon Sep 17 00:00:00 2001 From: Greg Greenway Date: Mon, 19 Jul 2021 10:08:19 -0700 Subject: [PATCH 01/12] Cleanup idle connection pools Delete connection pools when they have no connections anymore. This fixes unbounded memory use for cases where a new connection pool is needed for each downstream connection, such as when using upstream PROXY protocol. Fixes #16682 This reverts commit b7bc53945b4aefe4bff4d8fa498b3bf7933acd9e. This reverts PR #17319, by re-adding #17302 and #16948. Signed-off-by: Greg Greenway Co-authored-by: Craig Radcliffe --- .../other_features/ip_transparency.rst | 4 - docs/root/version_history/current.rst | 2 + envoy/common/conn_pool.h | 24 +- envoy/event/deferred_deletable.h | 6 + envoy/upstream/thread_local_cluster.h | 4 +- source/common/config/grpc_mux_impl.cc | 38 ++ source/common/config/grpc_mux_impl.h | 15 + source/common/config/new_grpc_mux_impl.cc | 41 +- source/common/config/new_grpc_mux_impl.h | 15 + source/common/conn_pool/conn_pool_base.cc | 50 ++- source/common/conn_pool/conn_pool_base.h | 23 +- source/common/event/dispatcher_impl.cc | 11 +- source/common/http/conn_pool_base.cc | 2 +- source/common/http/conn_pool_base.h | 7 +- source/common/http/conn_pool_grid.cc | 88 +++-- source/common/http/conn_pool_grid.h | 31 +- source/common/http/http1/conn_pool.cc | 2 +- source/common/runtime/runtime_features.cc | 1 + source/common/tcp/conn_pool.cc | 4 +- source/common/tcp/conn_pool.h | 7 +- source/common/tcp/original_conn_pool.cc | 41 +- source/common/tcp/original_conn_pool.h | 14 +- source/common/tcp_proxy/tcp_proxy.cc | 24 +- source/common/tcp_proxy/tcp_proxy.h | 1 + .../common/upstream/cluster_manager_impl.cc | 189 ++++++---- source/common/upstream/cluster_manager_impl.h | 16 +- source/common/upstream/conn_pool_map.h | 24 +- source/common/upstream/conn_pool_map_impl.h | 49 ++- .../common/upstream/priority_conn_pool_map.h | 20 +- .../upstream/priority_conn_pool_map_impl.h | 30 +- source/server/BUILD | 2 + source/server/server.cc | 6 + test/common/conn_pool/conn_pool_base_test.cc | 61 ++- test/common/http/conn_pool_grid_test.cc | 73 ++-- test/common/http/http1/conn_pool_test.cc | 15 +- test/common/http/http2/conn_pool_test.cc | 15 +- test/common/tcp/conn_pool_test.cc | 42 ++- .../upstream/cluster_manager_impl_test.cc | 351 ++++++++++++------ .../upstream/conn_pool_map_impl_test.cc | 65 ++-- .../priority_conn_pool_map_impl_test.cc | 35 +- .../idle_timeout_integration_test.cc | 16 + test/mocks/http/conn_pool.cc | 4 + test/mocks/http/conn_pool.h | 5 +- test/mocks/tcp/mocks.cc | 2 + test/mocks/tcp/mocks.h | 5 +- 45 files changed, 1080 insertions(+), 400 deletions(-) diff --git a/docs/root/intro/arch_overview/other_features/ip_transparency.rst b/docs/root/intro/arch_overview/other_features/ip_transparency.rst index 76ed11b5f5928..0e9fe81fc82d2 100644 --- a/docs/root/intro/arch_overview/other_features/ip_transparency.rst +++ b/docs/root/intro/arch_overview/other_features/ip_transparency.rst @@ -56,10 +56,6 @@ conjunction with the :ref:`Original Src Listener Filter `. Finally, Envoy supports generating this header using the :ref:`Proxy Protocol Transport Socket `. -IMPORTANT: There is currently a memory `issue `_ in Envoy where upstream connection pools are -not cleaned up after they are created. This heavily affects the usage of this transport socket as new pools are created for every downstream client -IP and port pair. Removing a cluster will clean up its associated connection pools, which could be used to mitigate this issue in the current state. - Here is an example config for setting up the socket: .. code-block:: yaml diff --git a/docs/root/version_history/current.rst b/docs/root/version_history/current.rst index cc6faa5ec0913..8d377c711d334 100644 --- a/docs/root/version_history/current.rst +++ b/docs/root/version_history/current.rst @@ -19,6 +19,8 @@ Bug Fixes --------- *Changes expected to improve the state of the world and are unlikely to have negative effects* +* cluster: delete pools when they're idle to fix unbounded memory use when using PROXY protocol upstream with tcp_proxy. This behavior can be temporarily reverted by setting the ``envoy.reloadable_features.conn_pool_delete_when_idle`` runtime guard to false. + Removed Config or Runtime ------------------------- *Normally occurs at the end of the* :ref:`deprecation period ` diff --git a/envoy/common/conn_pool.h b/envoy/common/conn_pool.h index e05664288b673..17cf77eca6824 100644 --- a/envoy/common/conn_pool.h +++ b/envoy/common/conn_pool.h @@ -44,17 +44,27 @@ class Instance { virtual ~Instance() = default; /** - * Called when a connection pool has been drained of pending streams, busy connections, and - * ready connections. + * Called when a connection pool has no pending streams, busy connections, or ready connections. */ - using DrainedCb = std::function; + using IdleCb = std::function; /** - * Register a callback that gets called when the connection pool is fully drained and kicks - * off a drain. The owner of the connection pool is responsible for not creating any - * new streams. + * Register a callback that gets called when the connection pool is fully idle. */ - virtual void addDrainedCallback(DrainedCb cb) PURE; + virtual void addIdleCallback(IdleCb cb) PURE; + + /** + * Returns true if the pool does not have any connections or pending requests. + */ + virtual bool isIdle() const PURE; + + /** + * Starts draining a pool, by gracefully completing all requests and gracefully closing all + * connections, in preparation for deletion. When the process completes, the function registered + * via `addIdleCallback()` is called. The callback may occur before this call returns if the pool + * can be immediately drained. + */ + virtual void startDrain() PURE; /** * Actively drain all existing connection pool connections. This method can be used in cases diff --git a/envoy/event/deferred_deletable.h b/envoy/event/deferred_deletable.h index c0e3dfee2835a..32b9398e322cd 100644 --- a/envoy/event/deferred_deletable.h +++ b/envoy/event/deferred_deletable.h @@ -13,6 +13,12 @@ namespace Event { class DeferredDeletable { public: virtual ~DeferredDeletable() = default; + + /** + * Called when an object is passed to `deferredDelete`. This signals that the object will soon + * be deleted. + */ + virtual void deleteIsPending() {} }; using DeferredDeletablePtr = std::unique_ptr; diff --git a/envoy/upstream/thread_local_cluster.h b/envoy/upstream/thread_local_cluster.h index 2efe626228475..16bba2855b0f3 100644 --- a/envoy/upstream/thread_local_cluster.h +++ b/envoy/upstream/thread_local_cluster.h @@ -31,9 +31,7 @@ class HttpPoolData { /** * See documentation of Envoy::ConnectionPool::Instance. */ - void addDrainedCallback(ConnectionPool::Instance::DrainedCb cb) { - pool_->addDrainedCallback(cb); - }; + void addIdleCallback(ConnectionPool::Instance::IdleCb cb) { pool_->addIdleCallback(cb); }; Upstream::HostDescriptionConstSharedPtr host() const { return pool_->host(); } diff --git a/source/common/config/grpc_mux_impl.cc b/source/common/config/grpc_mux_impl.cc index 58807df00d2d3..060f0c845796b 100644 --- a/source/common/config/grpc_mux_impl.cc +++ b/source/common/config/grpc_mux_impl.cc @@ -14,6 +14,35 @@ namespace Envoy { namespace Config { +namespace { +class AllMuxesState { +public: + void insert(GrpcMuxImpl* mux) { + absl::WriterMutexLock locker(&lock_); + muxes_.insert(mux); + } + + void erase(GrpcMuxImpl* mux) { + absl::WriterMutexLock locker(&lock_); + muxes_.erase(mux); + } + + void shutdownAll() { + absl::WriterMutexLock locker(&lock_); + for (auto& mux : muxes_) { + mux->shutdown(); + } + } + +private: + absl::flat_hash_set muxes_ ABSL_GUARDED_BY(lock_); + + // TODO(ggreenway): can this lock be removed? Is this code only run on the main thread? + absl::Mutex lock_; +}; +using AllMuxes = ThreadSafeSingleton; +} // namespace + GrpcMuxImpl::GrpcMuxImpl(const LocalInfo::LocalInfo& local_info, Grpc::RawAsyncClientPtr async_client, Event::Dispatcher& dispatcher, const Protobuf::MethodDescriptor& service_method, @@ -30,8 +59,13 @@ GrpcMuxImpl::GrpcMuxImpl(const LocalInfo::LocalInfo& local_info, onDynamicContextUpdate(resource_type_url); })) { Config::Utility::checkLocalInfo("ads", local_info); + AllMuxes::get().insert(this); } +GrpcMuxImpl::~GrpcMuxImpl() { AllMuxes::get().erase(this); } + +void GrpcMuxImpl::shutdownAll() { AllMuxes::get().shutdownAll(); } + void GrpcMuxImpl::onDynamicContextUpdate(absl::string_view resource_type_url) { auto api_state = api_state_.find(resource_type_url); if (api_state == api_state_.end()) { @@ -44,6 +78,10 @@ void GrpcMuxImpl::onDynamicContextUpdate(absl::string_view resource_type_url) { void GrpcMuxImpl::start() { grpc_stream_.establishNewStream(); } void GrpcMuxImpl::sendDiscoveryRequest(const std::string& type_url) { + if (shutdown_) { + return; + } + ApiState& api_state = apiStateFor(type_url); auto& request = api_state.request_; request.mutable_resource_names()->Clear(); diff --git a/source/common/config/grpc_mux_impl.h b/source/common/config/grpc_mux_impl.h index 585d028fe2b65..57b4946099373 100644 --- a/source/common/config/grpc_mux_impl.h +++ b/source/common/config/grpc_mux_impl.h @@ -39,6 +39,17 @@ class GrpcMuxImpl : public GrpcMux, Random::RandomGenerator& random, Stats::Scope& scope, const RateLimitSettings& rate_limit_settings, bool skip_subsequent_node); + ~GrpcMuxImpl() override; + + // Causes all GrpcMuxImpl objects to stop sending any messages on `grpc_stream_` to fix a crash + // on Envoy shutdown due to dangling pointers. This may not be the ideal fix; it is probably + // preferable for the `ServerImpl` to cause all configuration subscriptions to be shutdown, which + // would then cause all `GrpcMuxImpl` to be destructed. + // TODO: figure out the correct fix: https://github.com/envoyproxy/envoy/issues/15072. + static void shutdownAll(); + + void shutdown() { shutdown_ = true; } + void start() override; // GrpcMux @@ -179,6 +190,10 @@ class GrpcMuxImpl : public GrpcMux, Event::Dispatcher& dispatcher_; Common::CallbackHandlePtr dynamic_update_callback_handle_; + + // True iff Envoy is shutting down; no messages should be sent on the `grpc_stream_` when this is + // true because it may contain dangling pointers. + std::atomic shutdown_{false}; }; using GrpcMuxImplPtr = std::unique_ptr; diff --git a/source/common/config/new_grpc_mux_impl.cc b/source/common/config/new_grpc_mux_impl.cc index 89a829167e273..d0e3db537d0b9 100644 --- a/source/common/config/new_grpc_mux_impl.cc +++ b/source/common/config/new_grpc_mux_impl.cc @@ -16,6 +16,35 @@ namespace Envoy { namespace Config { +namespace { +class AllMuxesState { +public: + void insert(NewGrpcMuxImpl* mux) { + absl::WriterMutexLock locker(&lock_); + muxes_.insert(mux); + } + + void erase(NewGrpcMuxImpl* mux) { + absl::WriterMutexLock locker(&lock_); + muxes_.erase(mux); + } + + void shutdownAll() { + absl::WriterMutexLock locker(&lock_); + for (auto& mux : muxes_) { + mux->shutdown(); + } + } + +private: + absl::flat_hash_set muxes_ ABSL_GUARDED_BY(lock_); + + // TODO(ggreenway): can this lock be removed? Is this code only run on the main thread? + absl::Mutex lock_; +}; +using AllMuxes = ThreadSafeSingleton; +} // namespace + NewGrpcMuxImpl::NewGrpcMuxImpl(Grpc::RawAsyncClientPtr&& async_client, Event::Dispatcher& dispatcher, const Protobuf::MethodDescriptor& service_method, @@ -30,7 +59,13 @@ NewGrpcMuxImpl::NewGrpcMuxImpl(Grpc::RawAsyncClientPtr&& async_client, [this](absl::string_view resource_type_url) { onDynamicContextUpdate(resource_type_url); })), - transport_api_version_(transport_api_version), dispatcher_(dispatcher) {} + transport_api_version_(transport_api_version), dispatcher_(dispatcher) { + AllMuxes::get().insert(this); +} + +NewGrpcMuxImpl::~NewGrpcMuxImpl() { AllMuxes::get().erase(this); } + +void NewGrpcMuxImpl::shutdownAll() { AllMuxes::get().shutdownAll(); } void NewGrpcMuxImpl::onDynamicContextUpdate(absl::string_view resource_type_url) { auto sub = subscriptions_.find(resource_type_url); @@ -216,6 +251,10 @@ void NewGrpcMuxImpl::addSubscription(const std::string& type_url, const bool use } void NewGrpcMuxImpl::trySendDiscoveryRequests() { + if (shutdown_) { + return; + } + while (true) { // Do any of our subscriptions even want to send a request? absl::optional maybe_request_type = whoWantsToSendDiscoveryRequest(); diff --git a/source/common/config/new_grpc_mux_impl.h b/source/common/config/new_grpc_mux_impl.h index 4c2246fed813b..98ded0dec357b 100644 --- a/source/common/config/new_grpc_mux_impl.h +++ b/source/common/config/new_grpc_mux_impl.h @@ -38,6 +38,17 @@ class NewGrpcMuxImpl const RateLimitSettings& rate_limit_settings, const LocalInfo::LocalInfo& local_info); + ~NewGrpcMuxImpl() override; + + // Causes all NewGrpcMuxImpl objects to stop sending any messages on `grpc_stream_` to fix a crash + // on Envoy shutdown due to dangling pointers. This may not be the ideal fix; it is probably + // preferable for the `ServerImpl` to cause all configuration subscriptions to be shutdown, which + // would then cause all `NewGrpcMuxImpl` to be destructed. + // TODO: figure out the correct fix: https://github.com/envoyproxy/envoy/issues/15072. + static void shutdownAll(); + + void shutdown() { shutdown_ = true; } + GrpcMuxWatchPtr addWatch(const std::string& type_url, const absl::flat_hash_set& resources, SubscriptionCallbacks& callbacks, @@ -170,6 +181,10 @@ class NewGrpcMuxImpl Common::CallbackHandlePtr dynamic_update_callback_handle_; const envoy::config::core::v3::ApiVersion transport_api_version_; Event::Dispatcher& dispatcher_; + + // True iff Envoy is shutting down; no messages should be sent on the `grpc_stream_` when this is + // true because it may contain dangling pointers. + std::atomic shutdown_{false}; }; using NewGrpcMuxImplPtr = std::unique_ptr; diff --git a/source/common/conn_pool/conn_pool_base.cc b/source/common/conn_pool/conn_pool_base.cc index f83c9164126ee..76950f3ccde71 100644 --- a/source/common/conn_pool/conn_pool_base.cc +++ b/source/common/conn_pool/conn_pool_base.cc @@ -28,9 +28,13 @@ ConnPoolImplBase::ConnPoolImplBase( upstream_ready_cb_(dispatcher_.createSchedulableCallback([this]() { onUpstreamReady(); })) {} ConnPoolImplBase::~ConnPoolImplBase() { - ASSERT(ready_clients_.empty()); - ASSERT(busy_clients_.empty()); - ASSERT(connecting_clients_.empty()); + ASSERT(isIdleImpl()); + ASSERT(connecting_stream_capacity_ == 0); +} + +void ConnPoolImplBase::deleteIsPendingImpl() { + deferred_deleting_ = true; + ASSERT(isIdleImpl()); ASSERT(connecting_stream_capacity_ == 0); } @@ -225,6 +229,8 @@ void ConnPoolImplBase::onStreamClosed(Envoy::ConnectionPool::ActiveClient& clien } ConnectionPool::Cancellable* ConnPoolImplBase::newStream(AttachContext& context) { + ASSERT(!deferred_deleting_); + ASSERT(static_cast(connecting_stream_capacity_) == connectingCapacity(connecting_clients_)); // O(n) debug check. if (!ready_clients_.empty()) { @@ -272,6 +278,7 @@ ConnectionPool::Cancellable* ConnPoolImplBase::newStream(AttachContext& context) } bool ConnPoolImplBase::maybePreconnect(float global_preconnect_ratio) { + ASSERT(!deferred_deleting_); return tryCreateNewConnection(global_preconnect_ratio) == ConnectionResult::CreatedNewConnection; } @@ -322,9 +329,11 @@ void ConnPoolImplBase::transitionActiveClientState(ActiveClient& client, } } -void ConnPoolImplBase::addDrainedCallbackImpl(Instance::DrainedCb cb) { - drained_callbacks_.push_back(cb); - checkForDrained(); +void ConnPoolImplBase::addIdleCallbackImpl(Instance::IdleCb cb) { idle_callbacks_.push_back(cb); } + +void ConnPoolImplBase::startDrainImpl() { + is_draining_ = true; + checkForIdleAndCloseIdleConnsIfDraining(); } void ConnPoolImplBase::closeIdleConnectionsForDrainingPool() { @@ -366,17 +375,19 @@ void ConnPoolImplBase::drainConnectionsImpl() { } } -void ConnPoolImplBase::checkForDrained() { - if (drained_callbacks_.empty()) { - return; - } +bool ConnPoolImplBase::isIdleImpl() const { + return pending_streams_.empty() && ready_clients_.empty() && busy_clients_.empty() && + connecting_clients_.empty(); +} - closeIdleConnectionsForDrainingPool(); +void ConnPoolImplBase::checkForIdleAndCloseIdleConnsIfDraining() { + if (is_draining_) { + closeIdleConnectionsForDrainingPool(); + } - if (pending_streams_.empty() && ready_clients_.empty() && busy_clients_.empty() && - connecting_clients_.empty()) { - ENVOY_LOG(debug, "invoking drained callbacks"); - for (const Instance::DrainedCb& cb : drained_callbacks_) { + if (isIdleImpl()) { + ENVOY_LOG(debug, "invoking idle callbacks - is_draining_={}", is_draining_); + for (const Instance::IdleCb& cb : idle_callbacks_) { cb(); } } @@ -439,9 +450,8 @@ void ConnPoolImplBase::onConnectionEvent(ActiveClient& client, absl::string_view client.releaseResources(); dispatcher_.deferredDelete(client.removeFromList(owningList(client.state()))); - if (incomplete_stream) { - checkForDrained(); - } + + checkForIdleAndCloseIdleConnsIfDraining(); client.setState(ActiveClient::State::CLOSED); @@ -459,7 +469,7 @@ void ConnPoolImplBase::onConnectionEvent(ActiveClient& client, absl::string_view // refer to client after this point. onConnected(client); onUpstreamReady(); - checkForDrained(); + checkForIdleAndCloseIdleConnsIfDraining(); } } @@ -529,7 +539,7 @@ void ConnPoolImplBase::onPendingStreamCancel(PendingStream& stream, } host_->cluster().stats().upstream_rq_cancelled_.inc(); - checkForDrained(); + checkForIdleAndCloseIdleConnsIfDraining(); } namespace { diff --git a/source/common/conn_pool/conn_pool_base.h b/source/common/conn_pool/conn_pool_base.h index f4822b7e77f64..8e06e8e68ed2b 100644 --- a/source/common/conn_pool/conn_pool_base.h +++ b/source/common/conn_pool/conn_pool_base.h @@ -144,6 +144,8 @@ class ConnPoolImplBase : protected Logger::Loggable { Upstream::ClusterConnectivityState& state); virtual ~ConnPoolImplBase(); + void deleteIsPendingImpl(); + // A helper function to get the specific context type from the base class context. template T& typedContext(AttachContext& context) { ASSERT(dynamic_cast(&context) != nullptr); @@ -160,7 +162,8 @@ class ConnPoolImplBase : protected Logger::Loggable { int64_t connecting_and_connected_capacity, float preconnect_ratio, bool anticipate_incoming_stream = false); - void addDrainedCallbackImpl(Instance::DrainedCb cb); + void addIdleCallbackImpl(Instance::IdleCb cb); + void startDrainImpl(); void drainConnectionsImpl(); // Closes and destroys all connections. This must be called in the destructor of @@ -192,8 +195,13 @@ class ConnPoolImplBase : protected Logger::Loggable { void onConnectionEvent(ActiveClient& client, absl::string_view failure_reason, Network::ConnectionEvent event); - // See if the drain process has started and/or completed. - void checkForDrained(); + + // Returns true if the pool is idle. + bool isIdleImpl() const; + + // See if the pool has gone idle. If we're draining, this will also close idle connections. + void checkForIdleAndCloseIdleConnsIfDraining(); + void scheduleOnUpstreamReady(); ConnectionPool::Cancellable* newStream(AttachContext& context); // Called if this pool is likely to be picked soon, to determine if it's worth preconnecting. @@ -299,7 +307,7 @@ class ConnPoolImplBase : protected Logger::Loggable { const Network::ConnectionSocket::OptionsSharedPtr socket_options_; const Network::TransportSocketOptionsConstSharedPtr transport_socket_options_; - std::list drained_callbacks_; + std::list idle_callbacks_; // When calling purgePendingStreams, this list will be used to hold the streams we are about // to purge. We need this if one cancelled streams cancels a different pending stream @@ -325,6 +333,13 @@ class ConnPoolImplBase : protected Logger::Loggable { // The number of streams currently attached to clients. uint32_t num_active_streams_{0}; + // Whether the connection pool is currently in the process of closing + // all connections so that it can be gracefully deleted. + bool is_draining_{false}; + + // True iff this object is in the deferred delete list. + bool deferred_deleting_{false}; + void onUpstreamReady(); Event::SchedulableCallbackPtr upstream_ready_cb_; }; diff --git a/source/common/event/dispatcher_impl.cc b/source/common/event/dispatcher_impl.cc index f12399d2df666..ff1c855977b80 100644 --- a/source/common/event/dispatcher_impl.cc +++ b/source/common/event/dispatcher_impl.cc @@ -247,10 +247,13 @@ TimerPtr DispatcherImpl::createTimerInternal(TimerCb cb) { void DispatcherImpl::deferredDelete(DeferredDeletablePtr&& to_delete) { ASSERT(isThreadSafe()); - current_to_delete_->emplace_back(std::move(to_delete)); - ENVOY_LOG(trace, "item added to deferred deletion list (size={})", current_to_delete_->size()); - if (current_to_delete_->size() == 1) { - deferred_delete_cb_->scheduleCallbackCurrentIteration(); + if (to_delete != nullptr) { + to_delete->deleteIsPending(); + current_to_delete_->emplace_back(std::move(to_delete)); + ENVOY_LOG(trace, "item added to deferred deletion list (size={})", current_to_delete_->size()); + if (current_to_delete_->size() == 1) { + deferred_delete_cb_->scheduleCallbackCurrentIteration(); + } } } diff --git a/source/common/http/conn_pool_base.cc b/source/common/http/conn_pool_base.cc index 601295b8f9665..4a67bea9da859 100644 --- a/source/common/http/conn_pool_base.cc +++ b/source/common/http/conn_pool_base.cc @@ -141,7 +141,7 @@ void MultiplexedActiveClientBase::onStreamDestroy() { // wait until the connection has been fully drained of streams and then check in the connection // event callback. if (!closed_with_active_rq_) { - parent().checkForDrained(); + parent().checkForIdleAndCloseIdleConnsIfDraining(); } } diff --git a/source/common/http/conn_pool_base.h b/source/common/http/conn_pool_base.h index 0d72454786204..e2f7494c0e38b 100644 --- a/source/common/http/conn_pool_base.h +++ b/source/common/http/conn_pool_base.h @@ -55,8 +55,13 @@ class HttpConnPoolImplBase : public Envoy::ConnectionPool::ConnPoolImplBase, std::vector protocols); ~HttpConnPoolImplBase() override; + // Event::DeferredDeletable + void deleteIsPending() override { deleteIsPendingImpl(); } + // ConnectionPool::Instance - void addDrainedCallback(DrainedCb cb) override { addDrainedCallbackImpl(cb); } + void addIdleCallback(IdleCb cb) override { addIdleCallbackImpl(cb); } + bool isIdle() const override { return isIdleImpl(); } + void startDrain() override { startDrainImpl(); } void drainConnections() override { drainConnectionsImpl(); } Upstream::HostDescriptionConstSharedPtr host() const override { return host_; } ConnectionPool::Cancellable* newStream(Http::ResponseDecoder& response_decoder, diff --git a/source/common/http/conn_pool_grid.cc b/source/common/http/conn_pool_grid.cc index 28ce8d1c89c30..80f67419f8dea 100644 --- a/source/common/http/conn_pool_grid.cc +++ b/source/common/http/conn_pool_grid.cc @@ -205,7 +205,7 @@ ConnectivityGrid::ConnectivityGrid( } ConnectivityGrid::~ConnectivityGrid() { - // Ignore drained callbacks while the pools are destroyed below. + // Ignore idle callbacks while the pools are destroyed below. destroying_ = true; // Callbacks might have pending streams registered with the pools, so cancel and delete // the callback before deleting the pools. @@ -213,25 +213,40 @@ ConnectivityGrid::~ConnectivityGrid() { pools_.clear(); } +void ConnectivityGrid::deleteIsPending() { + deferred_deleting_ = true; + for (const auto& pool : pools_) { + pool->deleteIsPending(); + } +} + absl::optional ConnectivityGrid::createNextPool() { + ASSERT(!deferred_deleting_); // Pools are created by newStream, which should not be called during draining. - ASSERT(drained_callbacks_.empty()); + ASSERT(!draining_); // Right now, only H3 and TCP are supported, so if there are 2 pools we're done. - if (pools_.size() == 2 || !drained_callbacks_.empty()) { + if (pools_.size() == 2 || draining_) { return absl::nullopt; } // HTTP/3 is hard-coded as higher priority, H2 as secondary. + ConnectionPool::InstancePtr pool; if (pools_.empty()) { - pools_.push_back(Http3::allocateConnPool(dispatcher_, random_generator_, host_, priority_, - options_, transport_socket_options_, state_, - time_source_)); - return pools_.begin(); - } - pools_.push_back(std::make_unique(dispatcher_, random_generator_, host_, - priority_, options_, - transport_socket_options_, state_)); - return std::next(pools_.begin()); + pool = Http3::allocateConnPool(dispatcher_, random_generator_, host_, priority_, options_, + transport_socket_options_, state_, time_source_); + } else { + pool = std::make_unique(dispatcher_, random_generator_, host_, priority_, + options_, transport_socket_options_, state_); + } + + setupPool(*pool); + pools_.push_back(std::move(pool)); + + return --pools_.end(); +} + +void ConnectivityGrid::setupPool(ConnectionPool::Instance& pool) { + pool.addIdleCallback([this]() { onIdleReceived(); }); } bool ConnectivityGrid::hasActiveConnections() const { @@ -246,6 +261,11 @@ bool ConnectivityGrid::hasActiveConnections() const { ConnectionPool::Cancellable* ConnectivityGrid::newStream(Http::ResponseDecoder& decoder, ConnectionPool::Callbacks& callbacks) { + ASSERT(!deferred_deleting_); + + // New streams should not be created during draining. + ASSERT(!draining_); + if (pools_.empty()) { createNextPool(); } @@ -267,22 +287,24 @@ ConnectionPool::Cancellable* ConnectivityGrid::newStream(Http::ResponseDecoder& return ret; } -void ConnectivityGrid::addDrainedCallback(DrainedCb cb) { +void ConnectivityGrid::addIdleCallback(IdleCb cb) { // Add the callback to the list of callbacks to be called when all drains are // complete. - drained_callbacks_.emplace_back(cb); + idle_callbacks_.emplace_back(cb); +} - if (drained_callbacks_.size() != 1) { +void ConnectivityGrid::startDrain() { + if (draining_) { + // A drain callback has already been set, and only needs to happen once. return; } - // If this is the first time a drained callback has been added, track the - // number of pools which need to be drained in order to pass drain-completion - // up to the callers. Note that no new pools can be created from this point on - // as createNextPool fast-fails if drained callbacks are present. - drains_needed_ = pools_.size(); + // Note that no new pools can be created from this point on + // as createNextPool fast-fails if `draining_` is true. + draining_ = true; + for (auto& pool : pools_) { - pool->addDrainedCallback([this]() -> void { onDrainReceived(); }); + pool->startDrain(); } } @@ -316,21 +338,25 @@ void ConnectivityGrid::markHttp3Broken() { http3_status_tracker_.markHttp3Broken void ConnectivityGrid::markHttp3Confirmed() { http3_status_tracker_.markHttp3Confirmed(); } -void ConnectivityGrid::onDrainReceived() { - // Don't do any work under the stack of ~ConnectivityGrid() - if (destroying_) { - return; +bool ConnectivityGrid::isIdle() const { + // This is O(n) but the function is constant and there are no plans for n > 8. + bool idle = true; + for (const auto& pool : pools_) { + idle &= pool->isIdle(); } + return idle; +} - // If not all the pools have drained, keep waiting. - ASSERT(drains_needed_ != 0); - if (--drains_needed_ != 0) { +void ConnectivityGrid::onIdleReceived() { + // Don't do any work under the stack of ~ConnectivityGrid() + if (destroying_) { return; } - // All the pools have drained. Notify drain subscribers. - for (auto& callback : drained_callbacks_) { - callback(); + if (isIdle()) { + for (auto& callback : idle_callbacks_) { + callback(); + } } } diff --git a/source/common/http/conn_pool_grid.h b/source/common/http/conn_pool_grid.h index 5adf47dd4f7c3..f9ddd6d266e00 100644 --- a/source/common/http/conn_pool_grid.h +++ b/source/common/http/conn_pool_grid.h @@ -137,11 +137,16 @@ class ConnectivityGrid : public ConnectionPool::Instance, ConnectivityOptions connectivity_options); ~ConnectivityGrid() override; + // Event::DeferredDeletable + void deleteIsPending() override; + // Http::ConnPool::Instance bool hasActiveConnections() const override; ConnectionPool::Cancellable* newStream(Http::ResponseDecoder& response_decoder, ConnectionPool::Callbacks& callbacks) override; - void addDrainedCallback(DrainedCb cb) override; + void addIdleCallback(IdleCb cb) override; + bool isIdle() const override; + void startDrain() override; void drainConnections() override; Upstream::HostDescriptionConstSharedPtr host() const override; bool maybePreconnect(float preconnect_ratio) override; @@ -165,12 +170,16 @@ class ConnectivityGrid : public ConnectionPool::Instance, // event that HTTP/3 is marked broken again. void markHttp3Confirmed(); +protected: + // Set the required idle callback on the pool. + void setupPool(ConnectionPool::Instance& pool); + private: friend class ConnectivityGridForTest; - // Called by each pool as it drains. The grid is responsible for calling - // drained_callbacks_ once all pools have drained. - void onDrainReceived(); + // Called by each pool as it idles. The grid is responsible for calling + // idle_callbacks_ once all pools have idled. + void onIdleReceived(); // Returns true if HTTP/3 should be attempted because there is an alternate protocol // that specifies HTTP/3 and HTTP/3 is not broken. @@ -194,20 +203,24 @@ class ConnectivityGrid : public ConnectionPool::Instance, // TODO(RyanTheOptimist): Make the alternate_protocols_ member non-optional. AlternateProtocolsCacheSharedPtr alternate_protocols_; - // Tracks how many drains are needed before calling drain callbacks. This is - // set to the number of pools when the first drain callbacks are added, and - // decremented as various pools drain. - uint32_t drains_needed_ = 0; + // True iff this pool is draining. No new streams or connections should be created + // in this state. + bool draining_{false}; + // Tracks the callbacks to be called on drain completion. - std::list drained_callbacks_; + std::list idle_callbacks_; // The connection pools to use to create new streams, ordered in the order of // desired use. std::list pools_; + // True iff under the stack of the destructor, to avoid calling drain // callbacks on deletion. bool destroying_{}; + // True iff this pool is being being defer deleted. + bool deferred_deleting_{}; + // Wrapped callbacks are stashed in the wrapped_callbacks_ for ownership. std::list wrapped_callbacks_; }; diff --git a/source/common/http/http1/conn_pool.cc b/source/common/http/http1/conn_pool.cc index c7e9af0970108..87f33935474cf 100644 --- a/source/common/http/http1/conn_pool.cc +++ b/source/common/http/http1/conn_pool.cc @@ -63,7 +63,7 @@ void ActiveClient::StreamWrapper::onDecodeComplete() { pool->scheduleOnUpstreamReady(); parent_.stream_wrapper_.reset(); - pool->checkForDrained(); + pool->checkForIdleAndCloseIdleConnsIfDraining(); } } diff --git a/source/common/runtime/runtime_features.cc b/source/common/runtime/runtime_features.cc index 9d7782377a43b..74e6256751357 100644 --- a/source/common/runtime/runtime_features.cc +++ b/source/common/runtime/runtime_features.cc @@ -60,6 +60,7 @@ constexpr const char* runtime_features[] = { "envoy.reloadable_features.allow_response_for_timeout", "envoy.reloadable_features.check_unsupported_typed_per_filter_config", "envoy.reloadable_features.check_ocsp_policy", + "envoy.reloadable_features.conn_pool_delete_when_idle", "envoy.reloadable_features.dont_add_content_length_for_bodiless_requests", "envoy.reloadable_features.enable_compression_without_content_length_header", "envoy.reloadable_features.grpc_bridge_stats_disabled", diff --git a/source/common/tcp/conn_pool.cc b/source/common/tcp/conn_pool.cc index a4f33086956c6..38456bf4a5110 100644 --- a/source/common/tcp/conn_pool.cc +++ b/source/common/tcp/conn_pool.cc @@ -42,7 +42,7 @@ ActiveTcpClient::~ActiveTcpClient() { ASSERT(state() == ActiveClient::State::CLOSED); tcp_connection_data_->release(); parent_.onStreamClosed(*this, true); - parent_.checkForDrained(); + parent_.checkForIdleAndCloseIdleConnsIfDraining(); } } @@ -54,7 +54,7 @@ void ActiveTcpClient::clearCallbacks() { callbacks_ = nullptr; tcp_connection_data_ = nullptr; parent_.onStreamClosed(*this, true); - parent_.checkForDrained(); + parent_.checkForIdleAndCloseIdleConnsIfDraining(); } void ActiveTcpClient::onEvent(Network::ConnectionEvent event) { diff --git a/source/common/tcp/conn_pool.h b/source/common/tcp/conn_pool.h index e8c2f24c026e4..398254b498461 100644 --- a/source/common/tcp/conn_pool.h +++ b/source/common/tcp/conn_pool.h @@ -145,7 +145,12 @@ class ConnPoolImpl : public Envoy::ConnectionPool::ConnPoolImplBase, transport_socket_options, state) {} ~ConnPoolImpl() override { destructAllConnections(); } - void addDrainedCallback(DrainedCb cb) override { addDrainedCallbackImpl(cb); } + // Event::DeferredDeletable + void deleteIsPending() override { deleteIsPendingImpl(); } + + void addIdleCallback(IdleCb cb) override { addIdleCallbackImpl(cb); } + bool isIdle() const override { return isIdleImpl(); } + void startDrain() override { startDrainImpl(); } void drainConnections() override { drainConnectionsImpl(); // Legacy behavior for the TCP connection pool marks all connecting clients diff --git a/source/common/tcp/original_conn_pool.cc b/source/common/tcp/original_conn_pool.cc index 4f4da573b940d..cb4bf71b6735e 100644 --- a/source/common/tcp/original_conn_pool.cc +++ b/source/common/tcp/original_conn_pool.cc @@ -38,6 +38,7 @@ OriginalConnPoolImpl::~OriginalConnPoolImpl() { } void OriginalConnPoolImpl::drainConnections() { + ENVOY_LOG(debug, "draining connections"); while (!ready_conns_.empty()) { ready_conns_.front()->conn_->close(Network::ConnectionCloseType::NoFlush); } @@ -67,9 +68,11 @@ void OriginalConnPoolImpl::closeConnections() { } } -void OriginalConnPoolImpl::addDrainedCallback(DrainedCb cb) { - drained_callbacks_.push_back(cb); - checkForDrained(); +void OriginalConnPoolImpl::addIdleCallback(IdleCb cb) { idle_callbacks_.push_back(cb); } + +void OriginalConnPoolImpl::startDrain() { + is_draining_ = true; + checkForIdleAndCloseIdleConnsIfDraining(); } void OriginalConnPoolImpl::assignConnection(ActiveConn& conn, @@ -81,14 +84,22 @@ void OriginalConnPoolImpl::assignConnection(ActiveConn& conn, conn.real_host_description_); } -void OriginalConnPoolImpl::checkForDrained() { - if (!drained_callbacks_.empty() && pending_requests_.empty() && busy_conns_.empty() && - pending_conns_.empty()) { - while (!ready_conns_.empty()) { - ready_conns_.front()->conn_->close(Network::ConnectionCloseType::NoFlush); - } +bool OriginalConnPoolImpl::isIdle() const { + return pending_requests_.empty() && busy_conns_.empty() && pending_conns_.empty() && + ready_conns_.empty(); +} - for (const DrainedCb& cb : drained_callbacks_) { +void OriginalConnPoolImpl::checkForIdleAndCloseIdleConnsIfDraining() { + if (pending_requests_.empty() && busy_conns_.empty() && pending_conns_.empty() && + (is_draining_ || ready_conns_.empty())) { + if (is_draining_) { + ENVOY_LOG(debug, "in draining state"); + while (!ready_conns_.empty()) { + ready_conns_.front()->conn_->close(Network::ConnectionCloseType::NoFlush); + } + } + ENVOY_LOG(debug, "Calling idle callbacks - drained={}", is_draining_); + for (const IdleCb& cb : idle_callbacks_) { cb(); } } @@ -102,6 +113,8 @@ void OriginalConnPoolImpl::createNewConnection() { ConnectionPool::Cancellable* OriginalConnPoolImpl::newConnection(ConnectionPool::Callbacks& callbacks) { + ASSERT(!deferred_deleting_); + if (!ready_conns_.empty()) { ready_conns_.front()->moveBetweenLists(ready_conns_, busy_conns_); ENVOY_CONN_LOG(debug, "using existing connection", *busy_conns_.front()->conn_); @@ -196,8 +209,8 @@ void OriginalConnPoolImpl::onConnectionEvent(ActiveConn& conn, Network::Connecti createNewConnection(); } - if (check_for_drained) { - checkForDrained(); + if (check_for_drained || !is_draining_) { + checkForIdleAndCloseIdleConnsIfDraining(); } } @@ -232,7 +245,7 @@ void OriginalConnPoolImpl::onPendingRequestCancel(PendingRequest& request, pending_conns_.back()->conn_->close(Network::ConnectionCloseType::NoFlush); } - checkForDrained(); + checkForIdleAndCloseIdleConnsIfDraining(); } void OriginalConnPoolImpl::onConnReleased(ActiveConn& conn) { @@ -313,7 +326,7 @@ void OriginalConnPoolImpl::processIdleConnection(ActiveConn& conn, bool new_conn upstream_ready_cb_->scheduleCallbackCurrentIteration(); } - checkForDrained(); + checkForIdleAndCloseIdleConnsIfDraining(); } OriginalConnPoolImpl::ConnectionWrapper::ConnectionWrapper(ActiveConn& parent) : parent_(parent) { diff --git a/source/common/tcp/original_conn_pool.h b/source/common/tcp/original_conn_pool.h index b4c5f4bca5fd5..d5e79580c5b46 100644 --- a/source/common/tcp/original_conn_pool.h +++ b/source/common/tcp/original_conn_pool.h @@ -28,8 +28,13 @@ class OriginalConnPoolImpl : Logger::Loggable, public Connecti ~OriginalConnPoolImpl() override; + // Event::DeferredDeletable + void deleteIsPending() override { deferred_deleting_ = true; } + // ConnectionPool::Instance - void addDrainedCallback(DrainedCb cb) override; + void addIdleCallback(IdleCb cb) override; + bool isIdle() const override; + void startDrain() override; void drainConnections() override; void closeConnections() override; ConnectionPool::Cancellable* newConnection(ConnectionPool::Callbacks& callbacks) override; @@ -148,7 +153,7 @@ class OriginalConnPoolImpl : Logger::Loggable, public Connecti virtual void onConnDestroyed(ActiveConn& conn); void onUpstreamReady(); void processIdleConnection(ActiveConn& conn, bool new_connection, bool delay); - void checkForDrained(); + void checkForIdleAndCloseIdleConnsIfDraining(); Event::Dispatcher& dispatcher_; Upstream::HostConstSharedPtr host_; @@ -160,10 +165,13 @@ class OriginalConnPoolImpl : Logger::Loggable, public Connecti std::list ready_conns_; // conns ready for assignment std::list busy_conns_; // conns assigned std::list pending_requests_; - std::list drained_callbacks_; + std::list idle_callbacks_; Stats::TimespanPtr conn_connect_ms_; Event::SchedulableCallbackPtr upstream_ready_cb_; + bool upstream_ready_enabled_{false}; + bool is_draining_{false}; + bool deferred_deleting_{false}; }; } // namespace Tcp diff --git a/source/common/tcp_proxy/tcp_proxy.cc b/source/common/tcp_proxy/tcp_proxy.cc index ce3f18d7d65c2..a369af7358d16 100644 --- a/source/common/tcp_proxy/tcp_proxy.cc +++ b/source/common/tcp_proxy/tcp_proxy.cc @@ -711,14 +711,22 @@ void Filter::disableIdleTimer() { UpstreamDrainManager::~UpstreamDrainManager() { // If connections aren't closed before they are destructed an ASSERT fires, // so cancel all pending drains, which causes the connections to be closed. - while (!drainers_.empty()) { - auto begin = drainers_.begin(); - Drainer* key = begin->first; - begin->second->cancelDrain(); + if (!drainers_.empty()) { + auto& dispatcher = drainers_.begin()->second->dispatcher(); + while (!drainers_.empty()) { + auto begin = drainers_.begin(); + Drainer* key = begin->first; + begin->second->cancelDrain(); + + // cancelDrain() should cause that drainer to be removed from drainers_. + // ASSERT so that we don't end up in an infinite loop. + ASSERT(drainers_.find(key) == drainers_.end()); + } - // cancelDrain() should cause that drainer to be removed from drainers_. - // ASSERT so that we don't end up in an infinite loop. - ASSERT(drainers_.find(key) == drainers_.end()); + // This destructor is run when shutting down `ThreadLocal`. The destructor of some objects use + // earlier `ThreadLocal` slots (for accessing the runtime snapshot) so they must run before that + // slot is destructed. Clear the list to enforce that ordering. + dispatcher.clearDeferredDeleteList(); } } @@ -790,5 +798,7 @@ void Drainer::cancelDrain() { upstream_conn_data_->connection().close(Network::ConnectionCloseType::NoFlush); } +Event::Dispatcher& Drainer::dispatcher() { return upstream_conn_data_->connection().dispatcher(); } + } // namespace TcpProxy } // namespace Envoy diff --git a/source/common/tcp_proxy/tcp_proxy.h b/source/common/tcp_proxy/tcp_proxy.h index 06c5572a313f2..7e22c3273bc62 100644 --- a/source/common/tcp_proxy/tcp_proxy.h +++ b/source/common/tcp_proxy/tcp_proxy.h @@ -401,6 +401,7 @@ class Drainer : public Event::DeferredDeletable { void onIdleTimeout(); void onBytesSent(); void cancelDrain(); + Event::Dispatcher& dispatcher(); private: UpstreamDrainManager& parent_; diff --git a/source/common/upstream/cluster_manager_impl.cc b/source/common/upstream/cluster_manager_impl.cc index 63d0cf5c798fd..afc59e3c13e49 100644 --- a/source/common/upstream/cluster_manager_impl.cc +++ b/source/common/upstream/cluster_manager_impl.cc @@ -1116,6 +1116,9 @@ ClusterManagerImpl::ThreadLocalClusterManagerImpl::~ThreadLocalClusterManagerImp } } thread_local_clusters_.clear(); + + // Ensure that all pools are completely destructed. + thread_local_dispatcher_.clearDeferredDeleteList(); } void ClusterManagerImpl::ThreadLocalClusterManagerImpl::drainConnPools(const HostVector& hosts) { @@ -1129,7 +1132,7 @@ void ClusterManagerImpl::ThreadLocalClusterManagerImpl::drainConnPools(const Hos { auto container = host_tcp_conn_pool_map_.find(host); if (container != host_tcp_conn_pool_map_.end()) { - drainTcpConnPools(host, container->second); + drainTcpConnPools(container->second); } } } @@ -1137,80 +1140,37 @@ void ClusterManagerImpl::ThreadLocalClusterManagerImpl::drainConnPools(const Hos void ClusterManagerImpl::ThreadLocalClusterManagerImpl::drainConnPools( HostSharedPtr old_host, ConnPoolsContainer& container) { - container.drains_remaining_ += container.pools_->size(); - // Make a copy to protect against erasure in the callback. std::shared_ptr pools = container.pools_; - pools->addDrainedCallback([this, old_host]() -> void { - if (destroying_) { - // It is possible for a connection pool to fire drain callbacks during destruction. Instead - // of checking if old_host actually exists in the map, it's clearer and cleaner to keep - // track of destruction as a separate state and check for it here. This also allows us to - // do this check here versus inside every different connection pool implementation. - return; - } - - ConnPoolsContainer* to_clear = getHttpConnPoolsContainer(old_host); - if (to_clear == nullptr) { - // This could happen if we have cleaned out the host before iterating through every connection - // pool. Handle it by just continuing. - return; - } - - ASSERT(to_clear->drains_remaining_ > 0); - to_clear->drains_remaining_--; - if (to_clear->drains_remaining_ == 0 && to_clear->ready_to_drain_) { - clearContainer(old_host, *to_clear); - } - }); + container.draining_ = true; // We need to hold off on actually emptying out the container until we have finished processing - // `addDrainedCallback`. If we do not, then it's possible that the container could be erased in + // `addIdleCallback`. If we do not, then it's possible that the container could be erased in // the middle of its iteration, which leads to undefined behaviour. We handle that case by - // checking here to see if the drains have completed. - container.ready_to_drain_ = true; - if (container.drains_remaining_ == 0) { - clearContainer(old_host, container); - } -} + // guarding deletion with `do_not_delete_` in the registered idle callback, and then checking + // afterwards whether it is empty and deleting it if necessary. + container.do_not_delete_ = true; + pools->startDrain(); + container.do_not_delete_ = false; -void ClusterManagerImpl::ThreadLocalClusterManagerImpl::clearContainer( - HostSharedPtr old_host, ConnPoolsContainer& container) { - container.pools_->clear(); - host_http_conn_pool_map_.erase(old_host); + if (container.pools_->size() == 0) { + host_http_conn_pool_map_.erase(old_host); + } } void ClusterManagerImpl::ThreadLocalClusterManagerImpl::drainTcpConnPools( - HostSharedPtr old_host, TcpConnPoolsContainer& container) { - container.drains_remaining_ += container.pools_.size(); + TcpConnPoolsContainer& container) { + // Copy the pools so that it is safe for the completion callback to mutate `container.pools_`. + // `container` may be invalid after all calls to `startDrain()`. + std::vector pools; for (const auto& pair : container.pools_) { - pair.second->addDrainedCallback([this, old_host]() -> void { - if (destroying_) { - // It is possible for a connection pool to fire drain callbacks during destruction. Instead - // of checking if old_host actually exists in the map, it's clearer and cleaner to keep - // track of destruction as a separate state and check for it here. This also allows us to - // do this check here versus inside every different connection pool implementation. - return; - } - - TcpConnPoolsContainer& container = host_tcp_conn_pool_map_[old_host]; - ASSERT(container.drains_remaining_ > 0); - container.drains_remaining_--; - if (container.drains_remaining_ == 0) { - for (auto& pair : container.pools_) { - thread_local_dispatcher_.deferredDelete(std::move(pair.second)); - } - host_tcp_conn_pool_map_.erase(old_host); - } - }); + pools.push_back(pair.second.get()); + } - // The above addDrainedCallback() drain completion callback might execute immediately. This can - // then effectively nuke 'container', which means we can't continue to loop on its contents - // (we're done here). - if (host_tcp_conn_pool_map_.count(old_host) == 0) { - break; - } + container.draining_ = true; + for (auto pool : pools) { + pool->startDrain(); } } @@ -1271,7 +1231,13 @@ void ClusterManagerImpl::ThreadLocalClusterManagerImpl::onHostHealthFailure( { const auto container = getHttpConnPoolsContainer(host); if (container != nullptr) { + container->do_not_delete_ = true; container->pools_->drainConnections(); + container->do_not_delete_ = false; + + if (container->pools_->size() == 0) { + host_http_conn_pool_map_.erase(host); + } } } { @@ -1281,8 +1247,15 @@ void ClusterManagerImpl::ThreadLocalClusterManagerImpl::onHostHealthFailure( // active connections. const auto& container = host_tcp_conn_pool_map_.find(host); if (container != host_tcp_conn_pool_map_.end()) { + // Draining pools or closing connections can cause pool deletion if it becomes + // idle. Copy `pools_` so that we aren't iterating through a container that + // gets mutated by callbacks deleting from it. + std::vector pools; for (const auto& pair : container->second.pools_) { - const Tcp::ConnectionPool::InstancePtr& pool = pair.second; + pools.push_back(pair.second.get()); + } + + for (auto* pool : pools) { if (host->cluster().features() & ClusterInfo::Features::CLOSE_CONNECTIONS_ON_HOST_HEALTH_FAILURE) { pool->closeConnections(); @@ -1460,11 +1433,16 @@ ClusterManagerImpl::ThreadLocalClusterManagerImpl::ClusterEntry::connPool( // function. Otherwise, we'd need to capture a few of these variables by value. ConnPoolsContainer::ConnPools::PoolOptRef pool = container.pools_->getPool(priority, hash_key, [&]() { - return parent_.parent_.factory_.allocateConnPool( + auto pool = parent_.parent_.factory_.allocateConnPool( parent_.thread_local_dispatcher_, host, priority, upstream_protocols, alternate_protocol_options, !upstream_options->empty() ? upstream_options : nullptr, have_transport_socket_options ? context->upstreamTransportSocketOptions() : nullptr, parent_.parent_.time_source_, parent_.cluster_manager_state_); + + pool->addIdleCallback( + [this, host, priority, hash_key]() { httpConnPoolIsIdle(host, priority, hash_key); }); + + return pool; }); if (pool.has_value()) { @@ -1474,6 +1452,38 @@ ClusterManagerImpl::ThreadLocalClusterManagerImpl::ClusterEntry::connPool( } } +void ClusterManagerImpl::ThreadLocalClusterManagerImpl::ClusterEntry::httpConnPoolIsIdle( + HostConstSharedPtr host, ResourcePriority priority, const std::vector& hash_key) { + if (parent_.destroying_) { + // If the Cluster is being destroyed, this pool will be cleaned up by that + // process. + return; + } + + ConnPoolsContainer* container = parent_.getHttpConnPoolsContainer(host); + if (container == nullptr) { + // This could happen if we have cleaned out the host before iterating through every + // connection pool. Handle it by just continuing. + return; + } + + if (container->draining_ || + Runtime::runtimeFeatureEnabled("envoy.reloadable_features.conn_pool_delete_when_idle")) { + + ENVOY_LOG(trace, "Erasing idle pool for host {}", host); + container->pools_->erasePool(priority, hash_key); + + // Guard deletion of the container with `do_not_delete_` to avoid deletion while + // iterating through the container in `container->pools_->startDrain()`. See + // comment in `ClusterManagerImpl::ThreadLocalClusterManagerImpl::drainConnPools`. + if (!container->do_not_delete_ && container->pools_->size() == 0) { + ENVOY_LOG(trace, "Pool container empty for host {}, erasing host entry", host); + parent_.host_http_conn_pool_map_.erase( + host); // NOTE: `container` is erased after this point in the lambda. + } + } +} + Tcp::ConnectionPool::Instance* ClusterManagerImpl::ThreadLocalClusterManagerImpl::ClusterEntry::tcpConnPool( ResourcePriority priority, LoadBalancerContext* context, bool peek) { @@ -1511,15 +1521,50 @@ ClusterManagerImpl::ThreadLocalClusterManagerImpl::ClusterEntry::tcpConnPool( } TcpConnPoolsContainer& container = parent_.host_tcp_conn_pool_map_[host]; - if (!container.pools_[hash_key]) { - container.pools_[hash_key] = parent_.parent_.factory_.allocateTcpConnPool( - parent_.thread_local_dispatcher_, host, priority, - !upstream_options->empty() ? upstream_options : nullptr, - have_transport_socket_options ? context->upstreamTransportSocketOptions() : nullptr, - parent_.cluster_manager_state_); + auto pool_iter = container.pools_.find(hash_key); + if (pool_iter == container.pools_.end()) { + bool inserted; + std::tie(pool_iter, inserted) = container.pools_.emplace( + hash_key, + parent_.parent_.factory_.allocateTcpConnPool( + parent_.thread_local_dispatcher_, host, priority, + !upstream_options->empty() ? upstream_options : nullptr, + have_transport_socket_options ? context->upstreamTransportSocketOptions() : nullptr, + parent_.cluster_manager_state_)); + ASSERT(inserted); + pool_iter->second->addIdleCallback( + [this, host, hash_key]() { tcpConnPoolIsIdle(host, hash_key); }); + } + + return pool_iter->second.get(); +} + +void ClusterManagerImpl::ThreadLocalClusterManagerImpl::ClusterEntry::tcpConnPoolIsIdle( + HostConstSharedPtr host, const std::vector& hash_key) { + if (parent_.destroying_) { + // If the Cluster is being destroyed, this pool will be cleaned up by that process. + return; } - return container.pools_[hash_key].get(); + auto it = parent_.host_tcp_conn_pool_map_.find(host); + if (it != parent_.host_tcp_conn_pool_map_.end()) { + TcpConnPoolsContainer& container = it->second; + + auto erase_iter = container.pools_.find(hash_key); + if (erase_iter != container.pools_.end()) { + if (container.draining_ || + Runtime::runtimeFeatureEnabled("envoy.reloadable_features.conn_pool_delete_when_idle")) { + ENVOY_LOG(trace, "Idle pool, erasing pool for host {}", host); + parent_.thread_local_dispatcher_.deferredDelete(std::move(erase_iter->second)); + container.pools_.erase(erase_iter); + } + } + + if (container.pools_.empty()) { + parent_.host_tcp_conn_pool_map_.erase( + host); // NOTE: `container` is erased after this point in the lambda. + } + } } ClusterManagerPtr ProdClusterManagerFactory::clusterManagerFromProto( diff --git a/source/common/upstream/cluster_manager_impl.h b/source/common/upstream/cluster_manager_impl.h index c3d436eb3f9ac..a2c6a62e0fdd3 100644 --- a/source/common/upstream/cluster_manager_impl.h +++ b/source/common/upstream/cluster_manager_impl.h @@ -359,15 +359,18 @@ class ClusterManagerImpl : public ClusterManager, Logger::Loggable pools_; - bool ready_to_drain_{false}; - uint64_t drains_remaining_{}; + bool draining_{false}; + + // Protect from deletion while iterating through pools_. See comments and usage + // in `ClusterManagerImpl::ThreadLocalClusterManagerImpl::drainConnPools()`. + bool do_not_delete_{false}; }; struct TcpConnPoolsContainer { using ConnPools = std::map, Tcp::ConnectionPool::InstancePtr>; ConnPools pools_; - uint64_t drains_remaining_{}; + bool draining_{false}; }; // Holds an unowned reference to a connection, and watches for Closed events. If the connection @@ -409,6 +412,10 @@ class ClusterManagerImpl : public ClusterManager, Logger::Loggable& hash_key); + void tcpConnPoolIsIdle(HostConstSharedPtr host, const std::vector& hash_key); + // Upstream::ThreadLocalCluster const PrioritySet& prioritySet() override { return priority_set_; } ClusterInfoConstSharedPtr info() override { return cluster_info_; } @@ -445,8 +452,7 @@ class ClusterManagerImpl : public ClusterManager, Logger::Loggable class ConnPoolMap { public: using PoolFactory = std::function()>; - using DrainedCb = std::function; + using IdleCb = typename POOL_TYPE::IdleCb; using PoolOptRef = absl::optional>; ConnPoolMap(Event::Dispatcher& dispatcher, const HostConstSharedPtr& host, @@ -31,7 +31,14 @@ template class ConnPoolMap { * possible for this to fail if a limit on the number of pools allowed is reached. * @return The pool corresponding to `key`, or `absl::nullopt`. */ - PoolOptRef getPool(KEY_TYPE key, const PoolFactory& factory); + PoolOptRef getPool(const KEY_TYPE& key, const PoolFactory& factory); + + /** + * Erases an existing pool mapped to `key`. + * + * @return true if the entry exists and was removed, false otherwise + */ + bool erasePool(const KEY_TYPE& key); /** * @return the number of pools. @@ -44,15 +51,20 @@ template class ConnPoolMap { void clear(); /** - * Adds a drain callback to all mapped pools. Any future mapped pools with have the callback + * Adds an idle callback to all mapped pools. Any future mapped pools with have the callback * automatically added. Be careful with the callback. If it itself calls into `this`, modifying * the state of `this`, there is a good chance it will cause corruption due to the callback firing * immediately. */ - void addDrainedCallback(const DrainedCb& cb); + void addIdleCallback(const IdleCb& cb); + + /** + * See `Envoy::ConnectionPool::Instance::startDrain()`. + */ + void startDrain(); /** - * Instructs each connection pool to drain its connections. + * See `Envoy::ConnectionPool::Instance::drainConnections()`. */ void drainConnections(); @@ -70,7 +82,7 @@ template class ConnPoolMap { absl::flat_hash_map> active_pools_; Event::Dispatcher& thread_local_dispatcher_; - std::vector cached_callbacks_; + std::vector cached_callbacks_; Common::DebugRecursionChecker recursion_checker_; const HostConstSharedPtr host_; const ResourcePriority priority_; diff --git a/source/common/upstream/conn_pool_map_impl.h b/source/common/upstream/conn_pool_map_impl.h index b183f9d438bb7..18176d0a18ec8 100644 --- a/source/common/upstream/conn_pool_map_impl.h +++ b/source/common/upstream/conn_pool_map_impl.h @@ -21,7 +21,7 @@ template ConnPoolMap typename ConnPoolMap::PoolOptRef -ConnPoolMap::getPool(KEY_TYPE key, const PoolFactory& factory) { +ConnPoolMap::getPool(const KEY_TYPE& key, const PoolFactory& factory) { Common::AutoDebugRecursionChecker assert_not_in(recursion_checker_); // TODO(klarose): Consider how we will change the connection pool's configuration in the future. // The plan is to change the downstream socket options... We may want to take those as a parameter @@ -53,13 +53,28 @@ ConnPoolMap::getPool(KEY_TYPE key, const PoolFactory& facto auto new_pool = factory(); connPoolResource.inc(); for (const auto& cb : cached_callbacks_) { - new_pool->addDrainedCallback(cb); + new_pool->addIdleCallback(cb); } auto inserted = active_pools_.emplace(key, std::move(new_pool)); return std::ref(*inserted.first->second); } +template +bool ConnPoolMap::erasePool(const KEY_TYPE& key) { + Common::AutoDebugRecursionChecker assert_not_in(recursion_checker_); + auto pool_iter = active_pools_.find(key); + + if (pool_iter != active_pools_.end()) { + thread_local_dispatcher_.deferredDelete(std::move(pool_iter->second)); + active_pools_.erase(pool_iter); + host_->cluster().resourceManager(priority_).connectionPools().dec(); + return true; + } else { + return false; + } +} + template size_t ConnPoolMap::size() const { return active_pools_.size(); @@ -74,20 +89,42 @@ template void ConnPoolMap -void ConnPoolMap::addDrainedCallback(const DrainedCb& cb) { +void ConnPoolMap::addIdleCallback(const IdleCb& cb) { Common::AutoDebugRecursionChecker assert_not_in(recursion_checker_); for (auto& pool_pair : active_pools_) { - pool_pair.second->addDrainedCallback(cb); + pool_pair.second->addIdleCallback(cb); } cached_callbacks_.emplace_back(std::move(cb)); } +template +void ConnPoolMap::startDrain() { + // Copy the `active_pools_` so that it is safe for the call to result + // in deletion, and avoid iteration through a mutating container. + std::vector pools; + pools.reserve(active_pools_.size()); + for (auto& pool_pair : active_pools_) { + pools.push_back(pool_pair.second.get()); + } + + for (auto* pool : pools) { + pool->startDrain(); + } +} + template void ConnPoolMap::drainConnections() { - Common::AutoDebugRecursionChecker assert_not_in(recursion_checker_); + // Copy the `active_pools_` so that it is safe for the call to result + // in deletion, and avoid iteration through a mutating container. + std::vector pools; + pools.reserve(active_pools_.size()); for (auto& pool_pair : active_pools_) { - pool_pair.second->drainConnections(); + pools.push_back(pool_pair.second.get()); + } + + for (auto* pool : pools) { + pool->drainConnections(); } } diff --git a/source/common/upstream/priority_conn_pool_map.h b/source/common/upstream/priority_conn_pool_map.h index 18ce2c52eb959..c43ba46c06ea5 100644 --- a/source/common/upstream/priority_conn_pool_map.h +++ b/source/common/upstream/priority_conn_pool_map.h @@ -15,7 +15,7 @@ template class PriorityConnPoolMap { public: using ConnPoolMapType = ConnPoolMap; using PoolFactory = typename ConnPoolMapType::PoolFactory; - using DrainedCb = typename ConnPoolMapType::DrainedCb; + using IdleCb = typename ConnPoolMapType::IdleCb; using PoolOptRef = typename ConnPoolMapType::PoolOptRef; PriorityConnPoolMap(Event::Dispatcher& dispatcher, const HostConstSharedPtr& host); @@ -26,7 +26,12 @@ template class PriorityConnPoolMap { * is reached. * @return The pool corresponding to `key`, or `absl::nullopt`. */ - PoolOptRef getPool(ResourcePriority priority, KEY_TYPE key, const PoolFactory& factory); + PoolOptRef getPool(ResourcePriority priority, const KEY_TYPE& key, const PoolFactory& factory); + + /** + * Erase a pool for the given priority and `key` if it exists and is idle. + */ + bool erasePool(ResourcePriority priority, const KEY_TYPE& key); /** * @return the number of pools across all priorities. @@ -44,14 +49,21 @@ template class PriorityConnPoolMap { * the state of `this`, there is a good chance it will cause corruption due to the callback firing * immediately. */ - void addDrainedCallback(const DrainedCb& cb); + void addIdleCallback(const IdleCb& cb); /** - * Instructs each connection pool to drain its connections. + * See `Envoy::ConnectionPool::Instance::startDrain()`. + */ + void startDrain(); + + /** + * See `Envoy::ConnectionPool::Instance::drainConnections()`. */ void drainConnections(); private: + size_t getPriorityIndex(ResourcePriority priority) const; + std::array, NumResourcePriorities> conn_pool_maps_; }; diff --git a/source/common/upstream/priority_conn_pool_map_impl.h b/source/common/upstream/priority_conn_pool_map_impl.h index b1cb6f8c54d12..66cc9ff4407ea 100644 --- a/source/common/upstream/priority_conn_pool_map_impl.h +++ b/source/common/upstream/priority_conn_pool_map_impl.h @@ -20,11 +20,15 @@ PriorityConnPoolMap::~PriorityConnPoolMap() = default; template typename PriorityConnPoolMap::PoolOptRef -PriorityConnPoolMap::getPool(ResourcePriority priority, KEY_TYPE key, +PriorityConnPoolMap::getPool(ResourcePriority priority, const KEY_TYPE& key, const PoolFactory& factory) { - size_t index = static_cast(priority); - ASSERT(index < conn_pool_maps_.size()); - return conn_pool_maps_[index]->getPool(key, factory); + return conn_pool_maps_[getPriorityIndex(priority)]->getPool(key, factory); +} + +template +bool PriorityConnPoolMap::erasePool(ResourcePriority priority, + const KEY_TYPE& key) { + return conn_pool_maps_[getPriorityIndex(priority)]->erasePool(key); } template @@ -44,9 +48,16 @@ void PriorityConnPoolMap::clear() { } template -void PriorityConnPoolMap::addDrainedCallback(const DrainedCb& cb) { +void PriorityConnPoolMap::addIdleCallback(const IdleCb& cb) { for (auto& pool_map : conn_pool_maps_) { - pool_map->addDrainedCallback(cb); + pool_map->addIdleCallback(cb); + } +} + +template +void PriorityConnPoolMap::startDrain() { + for (auto& pool_map : conn_pool_maps_) { + pool_map->startDrain(); } } @@ -57,5 +68,12 @@ void PriorityConnPoolMap::drainConnections() { } } +template +size_t PriorityConnPoolMap::getPriorityIndex(ResourcePriority priority) const { + size_t index = static_cast(priority); + ASSERT(index < conn_pool_maps_.size()); + return index; +} + } // namespace Upstream } // namespace Envoy diff --git a/source/server/BUILD b/source/server/BUILD index 9c893e5444288..a632ff364c4ec 100644 --- a/source/server/BUILD +++ b/source/server/BUILD @@ -509,6 +509,8 @@ envoy_cc_library( "//source/common/common:logger_lib", "//source/common/common:mutex_tracer_lib", "//source/common/common:utility_lib", + "//source/common/config:grpc_mux_lib", + "//source/common/config:new_grpc_mux_lib", "//source/common/config:utility_lib", "//source/common/config:xds_resource_lib", "//source/common/grpc:async_client_manager_lib", diff --git a/source/server/server.cc b/source/server/server.cc index 81b06d0fcff84..58d675909caf5 100644 --- a/source/server/server.cc +++ b/source/server/server.cc @@ -29,6 +29,8 @@ #include "source/common/common/enum_to_int.h" #include "source/common/common/mutex_tracer_impl.h" #include "source/common/common/utility.h" +#include "source/common/config/grpc_mux_impl.h" +#include "source/common/config/new_grpc_mux_impl.h" #include "source/common/config/utility.h" #include "source/common/config/version_converter.h" #include "source/common/config/xds_resource.h" @@ -826,6 +828,10 @@ void InstanceImpl::terminate() { // Before the workers start exiting we should disable stat threading. stats_store_.shutdownThreading(); + // TODO: figure out the correct fix: https://github.com/envoyproxy/envoy/issues/15072. + Config::GrpcMuxImpl::shutdownAll(); + Config::NewGrpcMuxImpl::shutdownAll(); + if (overload_manager_) { overload_manager_->stop(); } diff --git a/test/common/conn_pool/conn_pool_base_test.cc b/test/common/conn_pool/conn_pool_base_test.cc index 5d8d64d359db3..f1f53f69f9fd8 100644 --- a/test/common/conn_pool/conn_pool_base_test.cc +++ b/test/common/conn_pool/conn_pool_base_test.cc @@ -60,7 +60,8 @@ class ConnPoolImplBaseTest : public testing::Test { // connection resource limit for most tests. cluster_->resetResourceManager(1024, 1024, 1024, 1, 1); ON_CALL(pool_, instantiateActiveClient).WillByDefault(Invoke([&]() -> ActiveClientPtr { - auto ret = std::make_unique(pool_, stream_limit_, concurrent_streams_); + auto ret = + std::make_unique>(pool_, stream_limit_, concurrent_streams_); clients_.push_back(ret.get()); ret->real_host_description_ = descr_; return ret; @@ -88,7 +89,7 @@ class ConnPoolImplBaseTest : public testing::Test { Upstream::makeTestHost(cluster_, "tcp://127.0.0.1:80", dispatcher_.timeSource())}; TestConnPoolImplBase pool_; AttachContext context_; - std::vector clients_; + std::vector clients_; }; TEST_F(ConnPoolImplBaseTest, DumpState) { @@ -201,5 +202,61 @@ TEST_F(ConnPoolImplBaseTest, ExplicitPreconnectNotHealthy) { EXPECT_FALSE(pool_.maybePreconnect(1)); } +// Remote close simulates the peer closing the connection. +TEST_F(ConnPoolImplBaseTest, PoolIdleCallbackTriggeredRemoteClose) { + EXPECT_CALL(dispatcher_, createTimer_(_)).Times(AnyNumber()); + + // Create a new stream using the pool + EXPECT_CALL(pool_, instantiateActiveClient); + pool_.newStream(context_); + ASSERT_EQ(1, clients_.size()); + + // Emulate the new upstream connection establishment + EXPECT_CALL(pool_, onPoolReady); + clients_.back()->onEvent(Network::ConnectionEvent::Connected); + + // The pool now has no requests/streams, but has an open connection, so it is not yet idle. + clients_.back()->active_streams_ = 0; + pool_.onStreamClosed(*clients_.back(), false); + + // Now that the last connection is closed, while there are no requests, the pool becomes idle. + testing::MockFunction idle_pool_callback; + EXPECT_CALL(idle_pool_callback, Call()); + pool_.addIdleCallbackImpl(idle_pool_callback.AsStdFunction()); + dispatcher_.clearDeferredDeleteList(); + clients_.back()->onEvent(Network::ConnectionEvent::RemoteClose); + + EXPECT_CALL(idle_pool_callback, Call()); + pool_.startDrainImpl(); +} + +// Local close simulates what would happen for an idle timeout on a connection. +TEST_F(ConnPoolImplBaseTest, PoolIdleCallbackTriggeredLocalClose) { + EXPECT_CALL(dispatcher_, createTimer_(_)).Times(AnyNumber()); + + // Create a new stream using the pool + EXPECT_CALL(pool_, instantiateActiveClient); + pool_.newStream(context_); + ASSERT_EQ(1, clients_.size()); + + // Emulate the new upstream connection establishment + EXPECT_CALL(pool_, onPoolReady); + clients_.back()->onEvent(Network::ConnectionEvent::Connected); + + // The pool now has no requests/streams, but has an open connection, so it is not yet idle. + clients_.back()->active_streams_ = 0; + pool_.onStreamClosed(*clients_.back(), false); + + // Now that the last connection is closed, while there are no requests, the pool becomes idle. + testing::MockFunction idle_pool_callback; + EXPECT_CALL(idle_pool_callback, Call()); + pool_.addIdleCallbackImpl(idle_pool_callback.AsStdFunction()); + dispatcher_.clearDeferredDeleteList(); + clients_.back()->onEvent(Network::ConnectionEvent::LocalClose); + + EXPECT_CALL(idle_pool_callback, Call()); + pool_.startDrainImpl(); +} + } // namespace ConnectionPool } // namespace Envoy diff --git a/test/common/http/conn_pool_grid_test.cc b/test/common/http/conn_pool_grid_test.cc index 945af22b86ab7..406bda423e3b5 100644 --- a/test/common/http/conn_pool_grid_test.cc +++ b/test/common/http/conn_pool_grid_test.cc @@ -41,6 +41,7 @@ class ConnectivityGridForTest : public ConnectivityGrid { return absl::nullopt; } ConnectionPool::MockInstance* instance = new NiceMock(); + setupPool(*instance); pools_.push_back(ConnectionPool::InstancePtr{instance}); ON_CALL(*instance, newStream(_, _)) .WillByDefault( @@ -401,32 +402,28 @@ TEST_F(ConnectivityGridTest, DrainCallbacks) { grid_.createNextPool(); bool drain_received = false; - bool second_drain_received = false; - ConnectionPool::Instance::DrainedCb pool1_cb; - ConnectionPool::Instance::DrainedCb pool2_cb; - // The first time a drained callback is added, the Grid's callback should be - // added to both pools. + grid_.addIdleCallback([&]() { drain_received = true; }); + + // The first time a drain is started, both pools should start draining. { - EXPECT_CALL(*grid_.first(), addDrainedCallback(_)) - .WillOnce(Invoke(Invoke([&](ConnectionPool::Instance::DrainedCb cb) { pool1_cb = cb; }))); - EXPECT_CALL(*grid_.second(), addDrainedCallback(_)) - .WillOnce(Invoke(Invoke([&](ConnectionPool::Instance::DrainedCb cb) { pool2_cb = cb; }))); - grid_.addDrainedCallback([&drain_received]() -> void { drain_received = true; }); + EXPECT_CALL(*grid_.first(), startDrain()); + EXPECT_CALL(*grid_.second(), startDrain()); + grid_.startDrain(); } - // The second time a drained callback is added, the pools will not see any - // change. + // The second time, the pools will not see any change. { - EXPECT_CALL(*grid_.first(), addDrainedCallback(_)).Times(0); - EXPECT_CALL(*grid_.second(), addDrainedCallback(_)).Times(0); - grid_.addDrainedCallback([&second_drain_received]() -> void { second_drain_received = true; }); + EXPECT_CALL(*grid_.first(), startDrain()).Times(0); + EXPECT_CALL(*grid_.second(), startDrain()).Times(0); + grid_.startDrain(); } { // Notify the grid the second pool has been drained. This should not be // passed up to the original callers. EXPECT_FALSE(drain_received); - (pool2_cb)(); + EXPECT_CALL(*grid_.second(), isIdle()).WillRepeatedly(Return(true)); + grid_.second()->idle_cb_(); EXPECT_FALSE(drain_received); } @@ -434,27 +431,57 @@ TEST_F(ConnectivityGridTest, DrainCallbacks) { // Notify the grid that another pool has been drained. Now that all pools are // drained, the original callers should be informed. EXPECT_FALSE(drain_received); - (pool1_cb)(); + EXPECT_CALL(*grid_.first(), isIdle()).WillRepeatedly(Return(true)); + grid_.first()->idle_cb_(); EXPECT_TRUE(drain_received); - EXPECT_TRUE(second_drain_received); } } +// Make sure idle callbacks work as expected. +TEST_F(ConnectivityGridTest, IdleCallbacks) { + // Synthetically create both pools. + grid_.createNextPool(); + grid_.createNextPool(); + + bool idle_received = false; + + grid_.addIdleCallback([&]() { idle_received = true; }); + EXPECT_FALSE(idle_received); + + // Notify the grid the second pool is idle. This should not be + // passed up to the original callers. + EXPECT_CALL(*grid_.second(), isIdle()).WillOnce(Return(true)); + EXPECT_CALL(*grid_.first(), isIdle()).WillOnce(Return(false)); + grid_.second()->idle_cb_(); + EXPECT_FALSE(idle_received); + + // Notify the grid that the first pool is idle, the but second no longer is. + EXPECT_CALL(*grid_.first(), isIdle()).WillOnce(Return(true)); + EXPECT_CALL(*grid_.second(), isIdle()).WillOnce(Return(false)); + grid_.first()->idle_cb_(); + EXPECT_FALSE(idle_received); + + // Notify the grid that both are now idle. This should be passed up + // to the original caller. + EXPECT_CALL(*grid_.first(), isIdle()).WillOnce(Return(true)); + EXPECT_CALL(*grid_.second(), isIdle()).WillOnce(Return(true)); + grid_.first()->idle_cb_(); + EXPECT_TRUE(idle_received); +} + // Ensure drain callbacks aren't called during grid teardown. TEST_F(ConnectivityGridTest, NoDrainOnTeardown) { grid_.createNextPool(); bool drain_received = false; - ConnectionPool::Instance::DrainedCb pool1_cb; { - EXPECT_CALL(*grid_.first(), addDrainedCallback(_)) - .WillOnce(Invoke(Invoke([&](ConnectionPool::Instance::DrainedCb cb) { pool1_cb = cb; }))); - grid_.addDrainedCallback([&drain_received]() -> void { drain_received = true; }); + grid_.addIdleCallback([&drain_received]() -> void { drain_received = true; }); + grid_.startDrain(); } grid_.setDestroying(); // Fake being in the destructor. - (pool1_cb)(); + grid_.first()->idle_cb_(); EXPECT_FALSE(drain_received); } diff --git a/test/common/http/http1/conn_pool_test.cc b/test/common/http/http1/conn_pool_test.cc index c0493c7307b47..f603967b14c97 100644 --- a/test/common/http/http1/conn_pool_test.cc +++ b/test/common/http/http1/conn_pool_test.cc @@ -31,6 +31,7 @@ #include "gtest/gtest.h" using testing::_; +using testing::AtLeast; using testing::DoAll; using testing::InSequence; using testing::Invoke; @@ -946,16 +947,17 @@ TEST_F(Http1ConnPoolImplTest, DrainCallback) { InSequence s; ReadyWatcher drained; - EXPECT_CALL(drained, ready()); - conn_pool_->addDrainedCallback([&]() -> void { drained.ready(); }); - ActiveTestRequest r1(*this, 0, ActiveTestRequest::Type::CreateConnection); ActiveTestRequest r2(*this, 0, ActiveTestRequest::Type::Pending); + + conn_pool_->addIdleCallback([&]() -> void { drained.ready(); }); + conn_pool_->startDrain(); + r2.handle_->cancel(Envoy::ConnectionPool::CancelPolicy::Default); EXPECT_EQ(1U, cluster_->stats_.upstream_rq_total_.value()); conn_pool_->expectEnableUpstreamReady(); - EXPECT_CALL(drained, ready()); + EXPECT_CALL(drained, ready()).Times(AtLeast(1)); r1.startRequest(); r1.completeResponse(false); @@ -975,10 +977,11 @@ TEST_F(Http1ConnPoolImplTest, DrainWhileConnecting) { Http::ConnectionPool::Cancellable* handle = conn_pool_->newStream(outer_decoder, callbacks); EXPECT_NE(nullptr, handle); - conn_pool_->addDrainedCallback([&]() -> void { drained.ready(); }); + conn_pool_->addIdleCallback([&]() -> void { drained.ready(); }); + conn_pool_->startDrain(); EXPECT_CALL(*conn_pool_->test_clients_[0].connection_, close(Network::ConnectionCloseType::NoFlush)); - EXPECT_CALL(drained, ready()); + EXPECT_CALL(drained, ready()).Times(AtLeast(1)); handle->cancel(Envoy::ConnectionPool::CancelPolicy::Default); EXPECT_CALL(*conn_pool_, onClientDestroy()); diff --git a/test/common/http/http2/conn_pool_test.cc b/test/common/http/http2/conn_pool_test.cc index 9ec7f843db07c..4f1b8e6f4bbec 100644 --- a/test/common/http/http2/conn_pool_test.cc +++ b/test/common/http/http2/conn_pool_test.cc @@ -23,6 +23,7 @@ #include "gtest/gtest.h" using testing::_; +using testing::AtLeast; using testing::DoAll; using testing::InSequence; using testing::Invoke; @@ -1089,7 +1090,8 @@ TEST_F(Http2ConnPoolImplTest, DrainDisconnectWithActiveRequest) { ->encodeHeaders(TestRequestHeaderMapImpl{{":path", "/"}, {":method", "GET"}}, true) .ok()); ReadyWatcher drained; - pool_->addDrainedCallback([&]() -> void { drained.ready(); }); + pool_->addIdleCallback([&]() -> void { drained.ready(); }); + pool_->startDrain(); EXPECT_CALL(dispatcher_, deferredDelete_(_)); EXPECT_CALL(drained, ready()); @@ -1125,7 +1127,8 @@ TEST_F(Http2ConnPoolImplTest, DrainDisconnectDrainingWithActiveRequest) { .ok()); ReadyWatcher drained; - pool_->addDrainedCallback([&]() -> void { drained.ready(); }); + pool_->addIdleCallback([&]() -> void { drained.ready(); }); + pool_->startDrain(); EXPECT_CALL(dispatcher_, deferredDelete_(_)); EXPECT_CALL(r2.decoder_, decodeHeaders_(_, true)); @@ -1168,7 +1171,8 @@ TEST_F(Http2ConnPoolImplTest, DrainPrimary) { .ok()); ReadyWatcher drained; - pool_->addDrainedCallback([&]() -> void { drained.ready(); }); + pool_->addIdleCallback([&]() -> void { drained.ready(); }); + pool_->startDrain(); EXPECT_CALL(dispatcher_, deferredDelete_(_)); EXPECT_CALL(r2.decoder_, decodeHeaders_(_, true)); @@ -1178,7 +1182,7 @@ TEST_F(Http2ConnPoolImplTest, DrainPrimary) { dispatcher_.clearDeferredDeleteList(); EXPECT_CALL(dispatcher_, deferredDelete_(_)); - EXPECT_CALL(drained, ready()); + EXPECT_CALL(drained, ready()).Times(AtLeast(1)); EXPECT_CALL(r1.decoder_, decodeHeaders_(_, true)); r1.inner_decoder_->decodeHeaders( ResponseHeaderMapPtr{new TestResponseHeaderMapImpl{{":status", "200"}}}, true); @@ -1223,7 +1227,8 @@ TEST_F(Http2ConnPoolImplTest, DrainPrimaryNoActiveRequest) { ReadyWatcher drained; EXPECT_CALL(drained, ready()); - pool_->addDrainedCallback([&]() -> void { drained.ready(); }); + pool_->addIdleCallback([&]() -> void { drained.ready(); }); + pool_->startDrain(); EXPECT_CALL(*this, onClientDestroy()); dispatcher_.clearDeferredDeleteList(); diff --git a/test/common/tcp/conn_pool_test.cc b/test/common/tcp/conn_pool_test.cc index c1febd25d1558..8657f444a4834 100644 --- a/test/common/tcp/conn_pool_test.cc +++ b/test/common/tcp/conn_pool_test.cc @@ -25,6 +25,7 @@ using testing::_; using testing::AnyNumber; +using testing::AtLeast; using testing::Invoke; using testing::InvokeWithoutArgs; using testing::NiceMock; @@ -104,7 +105,9 @@ class ConnPoolBase : public Tcp::ConnectionPool::Instance { Network::TransportSocketOptionsConstSharedPtr transport_socket_options, bool test_new_connection_pool); - void addDrainedCallback(DrainedCb cb) override { conn_pool_->addDrainedCallback(cb); } + void addIdleCallback(IdleCb cb) override { conn_pool_->addIdleCallback(cb); } + bool isIdle() const override { return conn_pool_->isIdle(); } + void startDrain() override { return conn_pool_->startDrain(); } void drainConnections() override { conn_pool_->drainConnections(); } void closeConnections() override { conn_pool_->closeConnections(); } ConnectionPool::Cancellable* newConnection(Tcp::ConnectionPool::Callbacks& callbacks) override { @@ -154,6 +157,7 @@ class ConnPoolBase : public Tcp::ConnectionPool::Instance { Event::MockDispatcher& mock_dispatcher_; NiceMock* mock_upstream_ready_cb_; std::vector test_conns_; + Upstream::HostSharedPtr host_; Network::ConnectionCallbacks* callbacks_ = nullptr; bool test_new_connection_pool_; Network::ConnectionSocket::OptionsSharedPtr options_; @@ -973,16 +977,16 @@ TEST_P(TcpConnPoolImplTest, ConnectionStateWithConcurrentConnections) { TEST_P(TcpConnPoolImplTest, DrainCallback) { initialize(); ReadyWatcher drained; - EXPECT_CALL(drained, ready()); - conn_pool_->addDrainedCallback([&]() -> void { drained.ready(); }); + conn_pool_->addIdleCallback([&]() -> void { drained.ready(); }); + conn_pool_->startDrain(); ActiveTestConn c1(*this, 0, ActiveTestConn::Type::CreateConnection); ActiveTestConn c2(*this, 0, ActiveTestConn::Type::Pending); c2.handle_->cancel(ConnectionPool::CancelPolicy::Default); EXPECT_CALL(*conn_pool_, onConnReleasedForTest()); - EXPECT_CALL(drained, ready()); + EXPECT_CALL(drained, ready()).Times(AtLeast(1)); c1.releaseConn(); EXPECT_CALL(*conn_pool_, onConnDestroyedForTest()); @@ -1002,11 +1006,13 @@ TEST_P(TcpConnPoolImplTest, DrainWhileConnecting) { Tcp::ConnectionPool::Cancellable* handle = conn_pool_->newConnection(callbacks); EXPECT_NE(nullptr, handle); - conn_pool_->addDrainedCallback([&]() -> void { drained.ready(); }); + conn_pool_->addIdleCallback([&]() -> void { drained.ready(); }); + conn_pool_->startDrain(); + if (test_new_connection_pool_) { // The shared connection pool removes and closes connecting clients if there are no // pending requests. - EXPECT_CALL(drained, ready()); + EXPECT_CALL(drained, ready()).Times(AtLeast(1)); handle->cancel(ConnectionPool::CancelPolicy::Default); } else { handle->cancel(ConnectionPool::CancelPolicy::Default); @@ -1026,11 +1032,12 @@ TEST_P(TcpConnPoolImplTest, DrainOnClose) { initialize(); ReadyWatcher drained; EXPECT_CALL(drained, ready()); - conn_pool_->addDrainedCallback([&]() -> void { drained.ready(); }); + conn_pool_->addIdleCallback([&]() -> void { drained.ready(); }); + conn_pool_->startDrain(); ActiveTestConn c1(*this, 0, ActiveTestConn::Type::CreateConnection); - EXPECT_CALL(drained, ready()); + EXPECT_CALL(drained, ready()).Times(AtLeast(1)); EXPECT_CALL(c1.callbacks_.callbacks_, onEvent(Network::ConnectionEvent::RemoteClose)) .WillOnce(Invoke([&](Network::ConnectionEvent event) -> void { EXPECT_EQ(Network::ConnectionEvent::RemoteClose, event); @@ -1111,6 +1118,25 @@ TEST_P(TcpConnPoolImplTest, RequestCapacity) { conn_pool_->test_conns_[2].connection_->raiseEvent(Network::ConnectionEvent::RemoteClose); } +// Test that connections that are closed due to idle timeout causes the idle callback to be fired. +TEST_P(TcpConnPoolImplTest, TestIdleTimeout) { + initialize(); + testing::MockFunction idle_callback; + conn_pool_->addIdleCallback(idle_callback.AsStdFunction()); + + EXPECT_CALL(idle_callback, Call()); + ActiveTestConn c1(*this, 0, ActiveTestConn::Type::CreateConnection); + EXPECT_CALL(*conn_pool_, onConnReleasedForTest()); + c1.releaseConn(); + conn_pool_->test_conns_[0].connection_->raiseEvent(Network::ConnectionEvent::RemoteClose); + + testing::MockFunction drained_callback; + EXPECT_CALL(idle_callback, Call()); + conn_pool_->startDrain(); + EXPECT_CALL(*conn_pool_, onConnDestroyedForTest()); + dispatcher_.clearDeferredDeleteList(); +} + // Test that maybePreconnect is passed up to the base class implementation. TEST_P(TcpConnPoolImplTest, TestPreconnect) { initialize(); diff --git a/test/common/upstream/cluster_manager_impl_test.cc b/test/common/upstream/cluster_manager_impl_test.cc index fa5f863ae53ff..e6edf516d24da 100644 --- a/test/common/upstream/cluster_manager_impl_test.cc +++ b/test/common/upstream/cluster_manager_impl_test.cc @@ -55,6 +55,8 @@ using ::testing::ReturnNew; using ::testing::ReturnRef; using ::testing::SaveArg; +using namespace std::chrono_literals; + envoy::config::bootstrap::v3::Bootstrap parseBootstrapFromV3Yaml(const std::string& yaml, bool avoid_boosting = true) { envoy::config::bootstrap::v3::Bootstrap bootstrap; @@ -1638,12 +1640,14 @@ TEST_F(ClusterManagerImplTest, DynamicAddRemove) { EXPECT_EQ(1UL, cluster_manager_->clusters().active_clusters_.size()); Http::ConnectionPool::MockInstance* cp = new Http::ConnectionPool::MockInstance(); EXPECT_CALL(factory_, allocateConnPool_(_, _, _, _, _)).WillOnce(Return(cp)); + EXPECT_CALL(*cp, addIdleCallback(_)); EXPECT_EQ(cp, HttpPoolDataPeer::getPool(cluster_manager_->getThreadLocalCluster("fake_cluster") ->httpConnPool(ResourcePriority::Default, Http::Protocol::Http11, nullptr))); Tcp::ConnectionPool::MockInstance* cp2 = new Tcp::ConnectionPool::MockInstance(); EXPECT_CALL(factory_, allocateTcpConnPool_(_)).WillOnce(Return(cp2)); + EXPECT_CALL(*cp2, addIdleCallback(_)); EXPECT_EQ(cp2, TcpPoolDataPeer::getPool(cluster_manager_->getThreadLocalCluster("fake_cluster") ->tcpConnPool(ResourcePriority::Default, nullptr))); @@ -1659,11 +1663,9 @@ TEST_F(ClusterManagerImplTest, DynamicAddRemove) { // Now remove the cluster. This should drain the connection pools, but not affect // tcp connections. - Http::ConnectionPool::Instance::DrainedCb drained_cb; - Tcp::ConnectionPool::Instance::DrainedCb drained_cb2; EXPECT_CALL(*callbacks, onClusterRemoval(_)); - EXPECT_CALL(*cp, addDrainedCallback(_)).WillOnce(SaveArg<0>(&drained_cb)); - EXPECT_CALL(*cp2, addDrainedCallback(_)).WillOnce(SaveArg<0>(&drained_cb2)); + EXPECT_CALL(*cp, startDrain()); + EXPECT_CALL(*cp2, startDrain()); EXPECT_TRUE(cluster_manager_->removeCluster("fake_cluster")); EXPECT_EQ(nullptr, cluster_manager_->getThreadLocalCluster("fake_cluster")); EXPECT_EQ(0UL, cluster_manager_->clusters().active_clusters_.size()); @@ -1676,9 +1678,6 @@ TEST_F(ClusterManagerImplTest, DynamicAddRemove) { // Remove an unknown cluster. EXPECT_FALSE(cluster_manager_->removeCluster("foo")); - drained_cb(); - drained_cb2(); - checkStats(1 /*added*/, 1 /*modified*/, 1 /*removed*/, 0 /*active*/, 0 /*warming*/); EXPECT_TRUE(Mock::VerifyAndClearExpectations(cluster1.get())); @@ -1778,8 +1777,8 @@ TEST_F(ClusterManagerImplTest, CloseHttpConnectionsOnHealthFailure) { Outlier::MockDetector outlier_detector; ON_CALL(*cluster1, outlierDetector()).WillByDefault(Return(&outlier_detector)); - Http::ConnectionPool::MockInstance* cp1 = new Http::ConnectionPool::MockInstance(); - Http::ConnectionPool::MockInstance* cp2 = new Http::ConnectionPool::MockInstance(); + Http::ConnectionPool::MockInstance* cp1 = new NiceMock(); + Http::ConnectionPool::MockInstance* cp2 = new NiceMock(); { InSequence s; @@ -1825,6 +1824,54 @@ TEST_F(ClusterManagerImplTest, CloseHttpConnectionsOnHealthFailure) { EXPECT_TRUE(Mock::VerifyAndClearExpectations(cluster1.get())); } +// Test that we close all HTTP connection pool connections when there is a host health failure. +// Verify that the pool gets deleted if it is idle, and that a crash does not occur due to +// deleting a container while iterating through it (see `do_not_delete_` in +// `ClusterManagerImpl::ThreadLocalClusterManagerImpl::onHostHealthFailure()`). +TEST_F(ClusterManagerImplTest, CloseHttpConnectionsAndDeletePoolOnHealthFailure) { + const std::string json = fmt::sprintf("{\"static_resources\":{%s}}", + clustersJson({defaultStaticClusterJson("some_cluster")})); + std::shared_ptr cluster1(new NiceMock()); + cluster1->info_->name_ = "some_cluster"; + HostSharedPtr test_host = makeTestHost(cluster1->info_, "tcp://127.0.0.1:80", time_system_); + cluster1->prioritySet().getMockHostSet(0)->hosts_ = {test_host}; + ON_CALL(*cluster1, initializePhase()).WillByDefault(Return(Cluster::InitializePhase::Primary)); + + MockHealthChecker health_checker; + ON_CALL(*cluster1, healthChecker()).WillByDefault(Return(&health_checker)); + + Outlier::MockDetector outlier_detector; + ON_CALL(*cluster1, outlierDetector()).WillByDefault(Return(&outlier_detector)); + + Http::ConnectionPool::MockInstance* cp1 = new NiceMock(); + + InSequence s; + + EXPECT_CALL(factory_, clusterFromProto_(_, _, _, _)) + .WillOnce(Return(std::make_pair(cluster1, nullptr))); + EXPECT_CALL(health_checker, addHostCheckCompleteCb(_)); + EXPECT_CALL(outlier_detector, addChangedStateCb(_)); + EXPECT_CALL(*cluster1, initialize(_)) + .WillOnce(Invoke([cluster1](std::function initialize_callback) { + // Test inline init. + initialize_callback(); + })); + create(parseBootstrapFromV3Json(json)); + + EXPECT_CALL(factory_, allocateConnPool_(_, _, _, _, _)).WillOnce(Return(cp1)); + cluster_manager_->getThreadLocalCluster("some_cluster") + ->httpConnPool(ResourcePriority::Default, Http::Protocol::Http11, nullptr); + + outlier_detector.runCallbacks(test_host); + health_checker.runCallbacks(test_host, HealthTransition::Unchanged); + + EXPECT_CALL(*cp1, drainConnections()).WillOnce(Invoke([&]() { cp1->idle_cb_(); })); + test_host->healthFlagSet(Host::HealthFlag::FAILED_OUTLIER_CHECK); + outlier_detector.runCallbacks(test_host); + + EXPECT_TRUE(Mock::VerifyAndClearExpectations(cluster1.get())); +} + // Test that we close all TCP connection pool connections when there is a host health failure. TEST_F(ClusterManagerImplTest, CloseTcpConnectionPoolsOnHealthFailure) { const std::string json = fmt::sprintf("{\"static_resources\":{%s}}", @@ -1841,8 +1888,8 @@ TEST_F(ClusterManagerImplTest, CloseTcpConnectionPoolsOnHealthFailure) { Outlier::MockDetector outlier_detector; ON_CALL(*cluster1, outlierDetector()).WillByDefault(Return(&outlier_detector)); - Tcp::ConnectionPool::MockInstance* cp1 = new Tcp::ConnectionPool::MockInstance(); - Tcp::ConnectionPool::MockInstance* cp2 = new Tcp::ConnectionPool::MockInstance(); + Tcp::ConnectionPool::MockInstance* cp1 = new NiceMock(); + Tcp::ConnectionPool::MockInstance* cp2 = new NiceMock(); { InSequence s; @@ -2072,7 +2119,7 @@ TEST_F(ClusterManagerImplTest, DynamicHostRemove) { EXPECT_CALL(factory_, allocateConnPool_(_, _, _, _, _)) .Times(4) - .WillRepeatedly(ReturnNew()); + .WillRepeatedly(ReturnNew>()); // This should provide us a CP for each of the above hosts. Http::ConnectionPool::MockInstance* cp1 = HttpPoolDataPeer::getPool( @@ -2092,14 +2139,9 @@ TEST_F(ClusterManagerImplTest, DynamicHostRemove) { EXPECT_NE(cp1_high, cp2_high); EXPECT_NE(cp1, cp1_high); - Http::ConnectionPool::Instance::DrainedCb drained_cb; - EXPECT_CALL(*cp1, addDrainedCallback(_)).WillOnce(SaveArg<0>(&drained_cb)); - Http::ConnectionPool::Instance::DrainedCb drained_cb_high; - EXPECT_CALL(*cp1_high, addDrainedCallback(_)).WillOnce(SaveArg<0>(&drained_cb_high)); - - EXPECT_CALL(factory_, allocateTcpConnPool_(_)) + EXPECT_CALL(factory_, allocateTcpConnPool_) .Times(4) - .WillRepeatedly(ReturnNew()); + .WillRepeatedly(ReturnNew>()); // This should provide us a CP for each of the above hosts. Tcp::ConnectionPool::MockInstance* tcp1 = @@ -2119,24 +2161,19 @@ TEST_F(ClusterManagerImplTest, DynamicHostRemove) { EXPECT_NE(tcp1_high, tcp2_high); EXPECT_NE(tcp1, tcp1_high); - Tcp::ConnectionPool::Instance::DrainedCb tcp_drained_cb; - EXPECT_CALL(*tcp1, addDrainedCallback(_)).WillOnce(SaveArg<0>(&tcp_drained_cb)); - Tcp::ConnectionPool::Instance::DrainedCb tcp_drained_cb_high; - EXPECT_CALL(*tcp1_high, addDrainedCallback(_)).WillOnce(SaveArg<0>(&tcp_drained_cb_high)); - // Remove the first host, this should lead to the first cp being drained. dns_timer_->invokeCallback(); dns_callback(Network::DnsResolver::ResolutionStatus::Success, TestUtility::makeDnsResponse({"127.0.0.2"})); - drained_cb(); - drained_cb = nullptr; - tcp_drained_cb(); - tcp_drained_cb = nullptr; - EXPECT_CALL(factory_.tls_.dispatcher_, deferredDelete_(_)).Times(4); - drained_cb_high(); - drained_cb_high = nullptr; - tcp_drained_cb_high(); - tcp_drained_cb_high = nullptr; + cp1->idle_cb_(); + cp1->idle_cb_ = nullptr; + tcp1->idle_cb_(); + tcp1->idle_cb_ = nullptr; + EXPECT_CALL(factory_.tls_.dispatcher_, deferredDelete_(_)).Times(2); + cp1_high->idle_cb_(); + cp1_high->idle_cb_ = nullptr; + tcp1_high->idle_cb_(); + tcp1_high->idle_cb_ = nullptr; // Make sure we get back the same connection pool for the 2nd host as we did before the change. Http::ConnectionPool::MockInstance* cp3 = HttpPoolDataPeer::getPool( @@ -2237,7 +2274,7 @@ TEST_F(ClusterManagerImplTest, DynamicHostRemoveWithTls) { EXPECT_CALL(factory_, allocateConnPool_(_, _, _, _, _)) .Times(4) - .WillRepeatedly(ReturnNew()); + .WillRepeatedly(ReturnNew>()); // This should provide us a CP for each of the above hosts. Http::ConnectionPool::MockInstance* cp1 = HttpPoolDataPeer::getPool( @@ -2257,14 +2294,9 @@ TEST_F(ClusterManagerImplTest, DynamicHostRemoveWithTls) { EXPECT_NE(cp1_high, cp2_high); EXPECT_NE(cp1, cp1_high); - Http::ConnectionPool::Instance::DrainedCb drained_cb; - EXPECT_CALL(*cp1, addDrainedCallback(_)).WillOnce(SaveArg<0>(&drained_cb)); - Http::ConnectionPool::Instance::DrainedCb drained_cb_high; - EXPECT_CALL(*cp1_high, addDrainedCallback(_)).WillOnce(SaveArg<0>(&drained_cb_high)); - - EXPECT_CALL(factory_, allocateTcpConnPool_(_)) + EXPECT_CALL(factory_, allocateTcpConnPool_) .Times(10) - .WillRepeatedly(ReturnNew()); + .WillRepeatedly(ReturnNew>()); // This should provide us a CP for each of the above hosts, and for different SNIs Tcp::ConnectionPool::MockInstance* tcp1 = @@ -2326,33 +2358,22 @@ TEST_F(ClusterManagerImplTest, DynamicHostRemoveWithTls) { EXPECT_CALL(factory_.tls_.dispatcher_, deferredDelete_(_)).Times(6); - Tcp::ConnectionPool::Instance::DrainedCb tcp_drained_cb; - EXPECT_CALL(*tcp1, addDrainedCallback(_)).WillOnce(SaveArg<0>(&tcp_drained_cb)); - Tcp::ConnectionPool::Instance::DrainedCb tcp_drained_cb_high; - EXPECT_CALL(*tcp1_high, addDrainedCallback(_)).WillOnce(SaveArg<0>(&tcp_drained_cb_high)); - - Tcp::ConnectionPool::Instance::DrainedCb tcp_drained_cb_example_com; - EXPECT_CALL(*tcp1_example_com, addDrainedCallback(_)) - .WillOnce(SaveArg<0>(&tcp_drained_cb_example_com)); - Tcp::ConnectionPool::Instance::DrainedCb tcp_drained_cb_ibm_com; - EXPECT_CALL(*tcp1_ibm_com, addDrainedCallback(_)).WillOnce(SaveArg<0>(&tcp_drained_cb_ibm_com)); - // Remove the first host, this should lead to the first cp being drained. dns_timer_->invokeCallback(); dns_callback(Network::DnsResolver::ResolutionStatus::Success, TestUtility::makeDnsResponse({"127.0.0.2"})); - drained_cb(); - drained_cb = nullptr; - tcp_drained_cb(); - tcp_drained_cb = nullptr; - drained_cb_high(); - drained_cb_high = nullptr; - tcp_drained_cb_high(); - tcp_drained_cb_high = nullptr; - tcp_drained_cb_example_com(); - tcp_drained_cb_example_com = nullptr; - tcp_drained_cb_ibm_com(); - tcp_drained_cb_ibm_com = nullptr; + cp1->idle_cb_(); + cp1->idle_cb_ = nullptr; + tcp1->idle_cb_(); + tcp1->idle_cb_ = nullptr; + cp1_high->idle_cb_(); + cp1_high->idle_cb_ = nullptr; + tcp1_high->idle_cb_(); + tcp1_high->idle_cb_ = nullptr; + tcp1_example_com->idle_cb_(); + tcp1_example_com->idle_cb_ = nullptr; + tcp1_ibm_com->idle_cb_(); + tcp1_ibm_com->idle_cb_ = nullptr; // Make sure we get back the same connection pool for the 2nd host as we did before the change. Http::ConnectionPool::MockInstance* cp3 = HttpPoolDataPeer::getPool( @@ -2813,10 +2834,10 @@ TEST_F(ClusterManagerImplTest, DynamicHostRemoveDefaultPriority) { TestUtility::makeDnsResponse({"127.0.0.2"})); EXPECT_CALL(factory_, allocateConnPool_(_, _, _, _, _)) - .WillOnce(ReturnNew()); + .WillOnce(ReturnNew>()); - EXPECT_CALL(factory_, allocateTcpConnPool_(_)) - .WillOnce(ReturnNew()); + EXPECT_CALL(factory_, allocateTcpConnPool_) + .WillOnce(ReturnNew>()); Http::ConnectionPool::MockInstance* cp = HttpPoolDataPeer::getPool( cluster_manager_->getThreadLocalCluster("cluster_1") @@ -2827,11 +2848,14 @@ TEST_F(ClusterManagerImplTest, DynamicHostRemoveDefaultPriority) { ->tcpConnPool(ResourcePriority::Default, nullptr)); // Immediate drain, since this can happen with the HTTP codecs. - EXPECT_CALL(*cp, addDrainedCallback(_)) - .WillOnce(Invoke([](Http::ConnectionPool::Instance::DrainedCb cb) { cb(); })); - - EXPECT_CALL(*tcp, addDrainedCallback(_)) - .WillOnce(Invoke([](Tcp::ConnectionPool::Instance::DrainedCb cb) { cb(); })); + EXPECT_CALL(*cp, startDrain()).WillOnce(Invoke([&]() { + cp->idle_cb_(); + cp->idle_cb_ = nullptr; + })); + EXPECT_CALL(*tcp, startDrain()).WillOnce(Invoke([&]() { + tcp->idle_cb_(); + tcp->idle_cb_ = nullptr; + })); // Remove the first host, this should lead to the cp being drained, without // crash. @@ -2900,24 +2924,25 @@ TEST_F(ClusterManagerImplTest, ConnPoolDestroyWithDraining) { TestUtility::makeDnsResponse({"127.0.0.2"})); MockConnPoolWithDestroy* mock_cp = new MockConnPoolWithDestroy(); + Http::ConnectionPool::Instance::IdleCb drained_cb; EXPECT_CALL(factory_, allocateConnPool_(_, _, _, _, _)).WillOnce(Return(mock_cp)); + EXPECT_CALL(*mock_cp, addIdleCallback(_)).WillOnce(SaveArg<0>(&drained_cb)); + EXPECT_CALL(*mock_cp, startDrain()); - MockTcpConnPoolWithDestroy* mock_tcp = new MockTcpConnPoolWithDestroy(); - EXPECT_CALL(factory_, allocateTcpConnPool_(_)).WillOnce(Return(mock_tcp)); + MockTcpConnPoolWithDestroy* mock_tcp = new NiceMock(); + Tcp::ConnectionPool::Instance::IdleCb tcp_drained_cb; + EXPECT_CALL(factory_, allocateTcpConnPool_).WillOnce(Return(mock_tcp)); + EXPECT_CALL(*mock_tcp, addIdleCallback(_)).WillOnce(SaveArg<0>(&tcp_drained_cb)); + EXPECT_CALL(*mock_tcp, startDrain()); - Http::ConnectionPool::MockInstance* cp = HttpPoolDataPeer::getPool( + HttpPoolDataPeer::getPool( cluster_manager_->getThreadLocalCluster("cluster_1") ->httpConnPool(ResourcePriority::Default, Http::Protocol::Http11, nullptr)); - Tcp::ConnectionPool::MockInstance* tcp = - TcpPoolDataPeer::getPool(cluster_manager_->getThreadLocalCluster("cluster_1") - ->tcpConnPool(ResourcePriority::Default, nullptr)); + TcpPoolDataPeer::getPool(cluster_manager_->getThreadLocalCluster("cluster_1") + ->tcpConnPool(ResourcePriority::Default, nullptr)); // Remove the first host, this should lead to the cp being drained. - Http::ConnectionPool::Instance::DrainedCb drained_cb; - EXPECT_CALL(*cp, addDrainedCallback(_)).WillOnce(SaveArg<0>(&drained_cb)); - Tcp::ConnectionPool::Instance::DrainedCb tcp_drained_cb; - EXPECT_CALL(*tcp, addDrainedCallback(_)).WillOnce(SaveArg<0>(&tcp_drained_cb)); dns_timer_->invokeCallback(); dns_callback(Network::DnsResolver::ResolutionStatus::Success, TestUtility::makeDnsResponse({})); @@ -3355,6 +3380,7 @@ TEST_F(ClusterManagerImplTest, UpstreamSocketOptionsPassedToTcpConnPool) { EXPECT_CALL(context, upstreamSocketOptions()).WillOnce(Return(options_to_return)); EXPECT_CALL(factory_, allocateTcpConnPool_(_)).WillOnce(Return(to_create)); + EXPECT_CALL(*to_create, addIdleCallback(_)); auto opt_cp = cluster_manager_->getThreadLocalCluster("cluster_1") ->tcpConnPool(ResourcePriority::Default, &context); @@ -3365,7 +3391,8 @@ TEST_F(ClusterManagerImplTest, UpstreamSocketOptionsPassedToConnPool) { createWithLocalClusterUpdate(); NiceMock context; - Http::ConnectionPool::MockInstance* to_create = new Http::ConnectionPool::MockInstance(); + Http::ConnectionPool::MockInstance* to_create = + new NiceMock(); Network::Socket::OptionsSharedPtr options_to_return = Network::SocketOptionFactory::buildIpTransparentOptions(); @@ -3382,8 +3409,10 @@ TEST_F(ClusterManagerImplTest, UpstreamSocketOptionsUsedInConnPoolHash) { NiceMock context1; NiceMock context2; - Http::ConnectionPool::MockInstance* to_create1 = new Http::ConnectionPool::MockInstance(); - Http::ConnectionPool::MockInstance* to_create2 = new Http::ConnectionPool::MockInstance(); + Http::ConnectionPool::MockInstance* to_create1 = + new NiceMock(); + Http::ConnectionPool::MockInstance* to_create2 = + new NiceMock(); Network::Socket::OptionsSharedPtr options1 = Network::SocketOptionFactory::buildIpTransparentOptions(); Network::Socket::OptionsSharedPtr options2 = @@ -3423,7 +3452,8 @@ TEST_F(ClusterManagerImplTest, UpstreamSocketOptionsNullIsOkay) { createWithLocalClusterUpdate(); NiceMock context; - Http::ConnectionPool::MockInstance* to_create = new Http::ConnectionPool::MockInstance(); + Http::ConnectionPool::MockInstance* to_create = + new NiceMock(); Network::Socket::OptionsSharedPtr options_to_return = nullptr; EXPECT_CALL(context, upstreamSocketOptions()).WillOnce(Return(options_to_return)); @@ -3442,6 +3472,7 @@ TEST_F(ClusterManagerImplTest, HttpPoolDataForwardsCallsToConnectionPool) { Network::Socket::OptionsSharedPtr options_to_return = nullptr; EXPECT_CALL(factory_, allocateConnPool_(_, _, _, _, _)).WillOnce(Return(pool_mock)); + EXPECT_CALL(*pool_mock, addIdleCallback(_)); auto opt_cp = cluster_manager_->getThreadLocalCluster("cluster_1") ->httpConnPool(ResourcePriority::Default, Http::Protocol::Http11, &context); @@ -3450,9 +3481,9 @@ TEST_F(ClusterManagerImplTest, HttpPoolDataForwardsCallsToConnectionPool) { EXPECT_CALL(*pool_mock, hasActiveConnections()).WillOnce(Return(true)); opt_cp.value().hasActiveConnections(); - ConnectionPool::Instance::DrainedCb drained_cb = []() {}; - EXPECT_CALL(*pool_mock, addDrainedCallback(_)); - opt_cp.value().addDrainedCallback(drained_cb); + ConnectionPool::Instance::IdleCb drained_cb = []() {}; + EXPECT_CALL(*pool_mock, addIdleCallback(_)); + opt_cp.value().addIdleCallback(drained_cb); } class TestUpstreamNetworkFilter : public Network::WriteFilter { @@ -4364,11 +4395,11 @@ TEST_F(ClusterManagerImplTest, ConnPoolsDrainedOnHostSetChange) { EXPECT_CALL(factory_, allocateConnPool_(_, _, _, _, _)) .Times(3) - .WillRepeatedly(ReturnNew()); + .WillRepeatedly(ReturnNew>()); - EXPECT_CALL(factory_, allocateTcpConnPool_(_)) + EXPECT_CALL(factory_, allocateTcpConnPool_) .Times(3) - .WillRepeatedly(ReturnNew()); + .WillRepeatedly(ReturnNew>()); // This should provide us a CP for each of the above hosts. Http::ConnectionPool::MockInstance* cp1 = HttpPoolDataPeer::getPool( @@ -4390,17 +4421,22 @@ TEST_F(ClusterManagerImplTest, ConnPoolsDrainedOnHostSetChange) { EXPECT_NE(cp1, cp2); EXPECT_NE(tcp1, tcp2); - EXPECT_CALL(*cp2, addDrainedCallback(_)) - .WillOnce(Invoke([](Http::ConnectionPool::Instance::DrainedCb cb) { cb(); })); - - EXPECT_CALL(*cp1, addDrainedCallback(_)) - .WillOnce(Invoke([](Http::ConnectionPool::Instance::DrainedCb cb) { cb(); })); - - EXPECT_CALL(*tcp1, addDrainedCallback(_)) - .WillOnce(Invoke([](Tcp::ConnectionPool::Instance::DrainedCb cb) { cb(); })); - - EXPECT_CALL(*tcp2, addDrainedCallback(_)) - .WillOnce(Invoke([](Tcp::ConnectionPool::Instance::DrainedCb cb) { cb(); })); + EXPECT_CALL(*cp2, startDrain()).WillOnce(Invoke([&]() { + cp2->idle_cb_(); + cp2->idle_cb_ = nullptr; + })); + EXPECT_CALL(*cp1, startDrain()).WillOnce(Invoke([&]() { + cp1->idle_cb_(); + cp1->idle_cb_ = nullptr; + })); + EXPECT_CALL(*tcp1, startDrain()).WillOnce(Invoke([&]() { + tcp1->idle_cb_(); + tcp1->idle_cb_ = nullptr; + })); + EXPECT_CALL(*tcp2, startDrain()).WillOnce(Invoke([&]() { + tcp2->idle_cb_(); + tcp2->idle_cb_ = nullptr; + })); HostVector hosts_removed; hosts_removed.push_back(host2); @@ -4423,11 +4459,14 @@ TEST_F(ClusterManagerImplTest, ConnPoolsDrainedOnHostSetChange) { HostVector hosts_added; hosts_added.push_back(host3); - EXPECT_CALL(*cp1, addDrainedCallback(_)) - .WillOnce(Invoke([](Http::ConnectionPool::Instance::DrainedCb cb) { cb(); })); - - EXPECT_CALL(*tcp1, addDrainedCallback(_)) - .WillOnce(Invoke([](Tcp::ConnectionPool::Instance::DrainedCb cb) { cb(); })); + EXPECT_CALL(*cp1, startDrain()).WillOnce(Invoke([&]() { + cp1->idle_cb_(); + cp1->idle_cb_ = nullptr; + })); + EXPECT_CALL(*tcp1, startDrain()).WillOnce(Invoke([&]() { + tcp1->idle_cb_(); + tcp1->idle_cb_ = nullptr; + })); // Adding host3 should drain connection pool for host1. cluster.prioritySet().updateHosts( @@ -4471,11 +4510,11 @@ TEST_F(ClusterManagerImplTest, ConnPoolsNotDrainedOnHostSetChange) { EXPECT_CALL(factory_, allocateConnPool_(_, _, _, _, _)) .Times(1) - .WillRepeatedly(ReturnNew()); + .WillRepeatedly(ReturnNew>()); - EXPECT_CALL(factory_, allocateTcpConnPool_(_)) + EXPECT_CALL(factory_, allocateTcpConnPool_) .Times(1) - .WillRepeatedly(ReturnNew()); + .WillRepeatedly(ReturnNew>()); // This should provide us a CP for each of the above hosts. Http::ConnectionPool::MockInstance* cp1 = HttpPoolDataPeer::getPool( @@ -4500,6 +4539,93 @@ TEST_F(ClusterManagerImplTest, ConnPoolsNotDrainedOnHostSetChange) { hosts_added, {}, 100); } +TEST_F(ClusterManagerImplTest, ConnPoolsIdleDeleted) { + const std::string yaml = R"EOF( + static_resources: + clusters: + - name: cluster_1 + connect_timeout: 0.25s + lb_policy: ROUND_ROBIN + type: STATIC + )EOF"; + + ReadyWatcher initialized; + EXPECT_CALL(initialized, ready()); + create(parseBootstrapFromV3Yaml(yaml)); + + // Set up for an initialize callback. + cluster_manager_->setInitializedCb([&]() -> void { initialized.ready(); }); + + std::unique_ptr callbacks(new NiceMock()); + ClusterUpdateCallbacksHandlePtr cb = + cluster_manager_->addThreadLocalClusterUpdateCallbacks(*callbacks); + + Cluster& cluster = cluster_manager_->activeClusters().begin()->second; + + // Set up the HostSet. + HostSharedPtr host1 = makeTestHost(cluster.info(), "tcp://127.0.0.1:80", time_system_); + + HostVector hosts{host1}; + auto hosts_ptr = std::make_shared(hosts); + + // Sending non-mergeable updates. + cluster.prioritySet().updateHosts( + 0, HostSetImpl::partitionHosts(hosts_ptr, HostsPerLocalityImpl::empty()), nullptr, hosts, {}, + 100); + + { + auto* cp1 = new NiceMock(); + EXPECT_CALL(factory_, allocateConnPool_(_, _, _, _, _)).WillOnce(Return(cp1)); + std::function idle_callback; + EXPECT_CALL(*cp1, addIdleCallback(_)).WillOnce(SaveArg<0>(&idle_callback)); + + EXPECT_EQ(cp1, HttpPoolDataPeer::getPool(cluster_manager_->getThreadLocalCluster("cluster_1") + ->httpConnPool(ResourcePriority::Default, + Http::Protocol::Http11, nullptr))); + // Request the same pool again and verify that it produces the same output + EXPECT_EQ(cp1, HttpPoolDataPeer::getPool(cluster_manager_->getThreadLocalCluster("cluster_1") + ->httpConnPool(ResourcePriority::Default, + Http::Protocol::Http11, nullptr))); + + // Trigger the idle callback so we remove the connection pool + idle_callback(); + + auto* cp2 = new NiceMock(); + EXPECT_CALL(factory_, allocateConnPool_(_, _, _, _, _)).WillOnce(Return(cp2)); + EXPECT_CALL(*cp2, addIdleCallback(_)); + + // This time we expect cp2 since cp1 will have been destroyed + EXPECT_EQ(cp2, HttpPoolDataPeer::getPool(cluster_manager_->getThreadLocalCluster("cluster_1") + ->httpConnPool(ResourcePriority::Default, + Http::Protocol::Http11, nullptr))); + } + + { + auto* tcp1 = new NiceMock(); + EXPECT_CALL(factory_, allocateTcpConnPool_).WillOnce(Return(tcp1)); + std::function idle_callback; + EXPECT_CALL(*tcp1, addIdleCallback(_)).WillOnce(SaveArg<0>(&idle_callback)); + EXPECT_EQ(tcp1, + TcpPoolDataPeer::getPool(cluster_manager_->getThreadLocalCluster("cluster_1") + ->tcpConnPool(ResourcePriority::Default, nullptr))); + // Request the same pool again and verify that it produces the same output + EXPECT_EQ(tcp1, + TcpPoolDataPeer::getPool(cluster_manager_->getThreadLocalCluster("cluster_1") + ->tcpConnPool(ResourcePriority::Default, nullptr))); + + // Trigger the idle callback so we remove the connection pool + idle_callback(); + + auto* tcp2 = new NiceMock(); + EXPECT_CALL(factory_, allocateTcpConnPool_).WillOnce(Return(tcp2)); + + // This time we expect tcp2 since tcp1 will have been destroyed + EXPECT_EQ(tcp2, + TcpPoolDataPeer::getPool(cluster_manager_->getThreadLocalCluster("cluster_1") + ->tcpConnPool(ResourcePriority::Default, nullptr))); + } +} + TEST_F(ClusterManagerImplTest, InvalidPriorityLocalClusterNameStatic) { std::string yaml = R"EOF( static_resources: @@ -4607,6 +4733,7 @@ TEST_F(ClusterManagerImplTest, ConnectionPoolPerDownstreamConnection) { std::vector conn_pool_vector; for (size_t i = 0; i < 3; ++i) { conn_pool_vector.push_back(new Http::ConnectionPool::MockInstance()); + EXPECT_CALL(*conn_pool_vector.back(), addIdleCallback(_)); EXPECT_CALL(factory_, allocateConnPool_(_, _, _, _, _)) .WillOnce(Return(conn_pool_vector.back())); EXPECT_CALL(downstream_connection, hashKey) @@ -4718,7 +4845,7 @@ TEST_F(PreconnectTest, PreconnectOn) { ->httpConnPool(ResourcePriority::Default, Http::Protocol::Http11, nullptr); http_handle.value().newStream(decoder_, http_callbacks_); - EXPECT_CALL(factory_, allocateTcpConnPool_(_)) + EXPECT_CALL(factory_, allocateTcpConnPool_) .Times(2) .WillRepeatedly(ReturnNew>()); auto tcp_handle = cluster_manager_->getThreadLocalCluster("cluster_1") diff --git a/test/common/upstream/conn_pool_map_impl_test.cc b/test/common/upstream/conn_pool_map_impl_test.cc index 6ce14d2becd6a..5b8cd99f2b78b 100644 --- a/test/common/upstream/conn_pool_map_impl_test.cc +++ b/test/common/upstream/conn_pool_map_impl_test.cc @@ -69,10 +69,10 @@ class ConnPoolMapImplTest : public testing::Test { }; } - TestMap::PoolFactory getFactoryExpectDrainedCb(Http::ConnectionPool::Instance::DrainedCb* cb) { + TestMap::PoolFactory getFactoryExpectIdleCb(Http::ConnectionPool::Instance::IdleCb* cb) { return [this, cb]() { auto pool = std::make_unique>(); - EXPECT_CALL(*pool, addDrainedCallback(_)).WillOnce(SaveArg<0>(cb)); + EXPECT_CALL(*pool, addIdleCallback(_)).WillOnce(SaveArg<0>(cb)); mock_pools_.push_back(pool.get()); return pool; }; @@ -153,13 +153,14 @@ TEST_F(ConnPoolMapImplTest, CallbacksPassedToPools) { test_map->getPool(1, getBasicFactory()); test_map->getPool(2, getBasicFactory()); - Http::ConnectionPool::Instance::DrainedCb cb1; - EXPECT_CALL(*mock_pools_[0], addDrainedCallback(_)).WillOnce(SaveArg<0>(&cb1)); - Http::ConnectionPool::Instance::DrainedCb cb2; - EXPECT_CALL(*mock_pools_[1], addDrainedCallback(_)).WillOnce(SaveArg<0>(&cb2)); + Http::ConnectionPool::Instance::IdleCb cb1; + EXPECT_CALL(*mock_pools_[0], addIdleCallback(_)).WillOnce(SaveArg<0>(&cb1)); + Http::ConnectionPool::Instance::IdleCb cb2; + EXPECT_CALL(*mock_pools_[1], addIdleCallback(_)).WillOnce(SaveArg<0>(&cb2)); ReadyWatcher watcher; - test_map->addDrainedCallback([&watcher] { watcher.ready(); }); + test_map->addIdleCallback([&watcher]() { watcher.ready(); }); + test_map->startDrain(); EXPECT_CALL(watcher, ready()).Times(2); cb1(); @@ -171,13 +172,14 @@ TEST_F(ConnPoolMapImplTest, CallbacksCachedAndPassedOnCreation) { TestMapPtr test_map = makeTestMap(); ReadyWatcher watcher; - test_map->addDrainedCallback([&watcher] { watcher.ready(); }); + test_map->addIdleCallback([&watcher]() { watcher.ready(); }); + test_map->startDrain(); - Http::ConnectionPool::Instance::DrainedCb cb1; - test_map->getPool(1, getFactoryExpectDrainedCb(&cb1)); + Http::ConnectionPool::Instance::IdleCb cb1; + test_map->getPool(1, getFactoryExpectIdleCb(&cb1)); - Http::ConnectionPool::Instance::DrainedCb cb2; - test_map->getPool(2, getFactoryExpectDrainedCb(&cb2)); + Http::ConnectionPool::Instance::IdleCb cb2; + test_map->getPool(2, getFactoryExpectIdleCb(&cb2)); EXPECT_CALL(watcher, ready()).Times(2); cb1(); @@ -205,7 +207,7 @@ TEST_F(ConnPoolMapImplTest, DrainConnectionsForwarded) { TEST_F(ConnPoolMapImplTest, ClearDefersDelete) { TestMapPtr test_map = makeTestMap(); - Http::ConnectionPool::Instance::DrainedCb cb1; + Http::ConnectionPool::Instance::IdleCb cb1; test_map->getPool(1, getBasicFactory()); test_map->getPool(2, getBasicFactory()); test_map->clear(); @@ -390,6 +392,19 @@ TEST_F(ConnPoolMapImplTest, CircuitBreakerUsesProvidedPriorityHigh) { test_map->getPool(2, getBasicFactory()); } +TEST_F(ConnPoolMapImplTest, ErasePool) { + TestMapPtr test_map = makeTestMap(); + auto* pool_ptr = &test_map->getPool(1, getBasicFactory()).value().get(); + EXPECT_EQ(1, test_map->size()); + EXPECT_EQ(pool_ptr, &test_map->getPool(1, getNeverCalledFactory()).value().get()); + EXPECT_EQ(1, test_map->size()); + EXPECT_FALSE(test_map->erasePool(2)); + EXPECT_EQ(1, test_map->size()); + EXPECT_TRUE(test_map->erasePool(1)); + EXPECT_EQ(0, test_map->size()); + EXPECT_NE(pool_ptr, &test_map->getPool(1, getBasicFactory()).value().get()); +} + // The following tests only die in debug builds, so don't run them if this isn't one. #if !defined(NDEBUG) class ConnPoolMapImplDeathTest : public ConnPoolMapImplTest {}; @@ -398,10 +413,10 @@ TEST_F(ConnPoolMapImplDeathTest, ReentryClearTripsAssert) { TestMapPtr test_map = makeTestMap(); test_map->getPool(1, getBasicFactory()); - ON_CALL(*mock_pools_[0], addDrainedCallback(_)) - .WillByDefault(Invoke([](Http::ConnectionPool::Instance::DrainedCb cb) { cb(); })); + ON_CALL(*mock_pools_[0], addIdleCallback(_)) + .WillByDefault(Invoke([](Http::ConnectionPool::Instance::IdleCb cb) { cb(); })); - EXPECT_DEATH(test_map->addDrainedCallback([&test_map] { test_map->clear(); }), + EXPECT_DEATH(test_map->addIdleCallback([&test_map]() { test_map->clear(); }), ".*Details: A resource should only be entered once"); } @@ -409,11 +424,11 @@ TEST_F(ConnPoolMapImplDeathTest, ReentryGetPoolTripsAssert) { TestMapPtr test_map = makeTestMap(); test_map->getPool(1, getBasicFactory()); - ON_CALL(*mock_pools_[0], addDrainedCallback(_)) - .WillByDefault(Invoke([](Http::ConnectionPool::Instance::DrainedCb cb) { cb(); })); + ON_CALL(*mock_pools_[0], addIdleCallback(_)) + .WillByDefault(Invoke([](Http::ConnectionPool::Instance::IdleCb cb) { cb(); })); EXPECT_DEATH( - test_map->addDrainedCallback([&test_map, this] { test_map->getPool(2, getBasicFactory()); }), + test_map->addIdleCallback([&test_map, this]() { test_map->getPool(2, getBasicFactory()); }), ".*Details: A resource should only be entered once"); } @@ -421,10 +436,10 @@ TEST_F(ConnPoolMapImplDeathTest, ReentryDrainConnectionsTripsAssert) { TestMapPtr test_map = makeTestMap(); test_map->getPool(1, getBasicFactory()); - ON_CALL(*mock_pools_[0], addDrainedCallback(_)) - .WillByDefault(Invoke([](Http::ConnectionPool::Instance::DrainedCb cb) { cb(); })); + ON_CALL(*mock_pools_[0], addIdleCallback(_)) + .WillByDefault(Invoke([](Http::ConnectionPool::Instance::IdleCb cb) { cb(); })); - EXPECT_DEATH(test_map->addDrainedCallback([&test_map] { test_map->drainConnections(); }), + EXPECT_DEATH(test_map->addIdleCallback([&test_map]() { test_map->clear(); }), ".*Details: A resource should only be entered once"); } @@ -432,10 +447,10 @@ TEST_F(ConnPoolMapImplDeathTest, ReentryAddDrainedCallbackTripsAssert) { TestMapPtr test_map = makeTestMap(); test_map->getPool(1, getBasicFactory()); - ON_CALL(*mock_pools_[0], addDrainedCallback(_)) - .WillByDefault(Invoke([](Http::ConnectionPool::Instance::DrainedCb cb) { cb(); })); + ON_CALL(*mock_pools_[0], addIdleCallback(_)) + .WillByDefault(Invoke([](Http::ConnectionPool::Instance::IdleCb cb) { cb(); })); - EXPECT_DEATH(test_map->addDrainedCallback([&test_map] { test_map->addDrainedCallback([]() {}); }), + EXPECT_DEATH(test_map->addIdleCallback([&test_map]() { test_map->addIdleCallback([]() {}); }), ".*Details: A resource should only be entered once"); } #endif // !defined(NDEBUG) diff --git a/test/common/upstream/priority_conn_pool_map_impl_test.cc b/test/common/upstream/priority_conn_pool_map_impl_test.cc index 60252e332c58c..a7ade68348547 100644 --- a/test/common/upstream/priority_conn_pool_map_impl_test.cc +++ b/test/common/upstream/priority_conn_pool_map_impl_test.cc @@ -36,6 +36,13 @@ class PriorityConnPoolMapImplTest : public testing::Test { }; } + TestMap::PoolFactory getNeverCalledFactory() { + return []() { + EXPECT_TRUE(false); + return nullptr; + }; + } + protected: NiceMock dispatcher_; std::vector*> mock_pools_; @@ -104,6 +111,24 @@ TEST_F(PriorityConnPoolMapImplTest, TestClearEmptiesOut) { EXPECT_EQ(test_map->size(), 0); } +TEST_F(PriorityConnPoolMapImplTest, TestErase) { + TestMapPtr test_map = makeTestMap(); + + auto* pool_ptr = &test_map->getPool(ResourcePriority::High, 1, getBasicFactory()).value().get(); + EXPECT_EQ(1, test_map->size()); + EXPECT_EQ(pool_ptr, + &test_map->getPool(ResourcePriority::High, 1, getNeverCalledFactory()).value().get()); + EXPECT_FALSE(test_map->erasePool(ResourcePriority::Default, 1)); + EXPECT_NE(pool_ptr, + &test_map->getPool(ResourcePriority::Default, 1, getBasicFactory()).value().get()); + EXPECT_EQ(2, test_map->size()); + EXPECT_TRUE(test_map->erasePool(ResourcePriority::Default, 1)); + EXPECT_TRUE(test_map->erasePool(ResourcePriority::High, 1)); + EXPECT_EQ(0, test_map->size()); + EXPECT_NE(pool_ptr, + &test_map->getPool(ResourcePriority::High, 1, getBasicFactory()).value().get()); +} + // Show that the drained callback is invoked once for the high priority pool, and once for // the default priority pool. TEST_F(PriorityConnPoolMapImplTest, TestAddDrainedCbProxiedThrough) { @@ -112,13 +137,13 @@ TEST_F(PriorityConnPoolMapImplTest, TestAddDrainedCbProxiedThrough) { test_map->getPool(ResourcePriority::High, 0, getBasicFactory()); test_map->getPool(ResourcePriority::Default, 0, getBasicFactory()); - Http::ConnectionPool::Instance::DrainedCb cbHigh; - EXPECT_CALL(*mock_pools_[0], addDrainedCallback(_)).WillOnce(SaveArg<0>(&cbHigh)); - Http::ConnectionPool::Instance::DrainedCb cbDefault; - EXPECT_CALL(*mock_pools_[1], addDrainedCallback(_)).WillOnce(SaveArg<0>(&cbDefault)); + Http::ConnectionPool::Instance::IdleCb cbHigh; + EXPECT_CALL(*mock_pools_[0], addIdleCallback(_)).WillOnce(SaveArg<0>(&cbHigh)); + Http::ConnectionPool::Instance::IdleCb cbDefault; + EXPECT_CALL(*mock_pools_[1], addIdleCallback(_)).WillOnce(SaveArg<0>(&cbDefault)); ReadyWatcher watcher; - test_map->addDrainedCallback([&watcher] { watcher.ready(); }); + test_map->addIdleCallback([&watcher]() { watcher.ready(); }); EXPECT_CALL(watcher, ready()).Times(2); cbHigh(); diff --git a/test/integration/idle_timeout_integration_test.cc b/test/integration/idle_timeout_integration_test.cc index 9a8e0f75e1972..9ba8b0ff29ebe 100644 --- a/test/integration/idle_timeout_integration_test.cc +++ b/test/integration/idle_timeout_integration_test.cc @@ -105,6 +105,16 @@ TEST_P(IdleTimeoutIntegrationTest, TimeoutBasic) { idle_time_out->set_seconds(seconds.count()); ConfigHelper::setProtocolOptions(*bootstrap.mutable_static_resources()->mutable_clusters(0), protocol_options); + + // Set pool limit so that the test can use it's stats to validate that + // the pool is deleted. + envoy::config::cluster::v3::CircuitBreakers circuit_breakers; + auto* threshold = circuit_breakers.mutable_thresholds()->Add(); + threshold->mutable_max_connection_pools()->set_value(1); + bootstrap.mutable_static_resources() + ->mutable_clusters(0) + ->mutable_circuit_breakers() + ->MergeFrom(circuit_breakers); }); initialize(); @@ -112,6 +122,9 @@ TEST_P(IdleTimeoutIntegrationTest, TimeoutBasic) { auto response = codec_client_->makeRequestWithBody(default_request_headers_, 1024); waitForNextUpstreamRequest(); + // Validate that the circuit breaker config is setup as we expect. + test_server_->waitForGaugeEq("cluster.cluster_0.circuit_breakers.default.cx_pool_open", 1); + upstream_request_->encodeHeaders(default_response_headers_, false); upstream_request_->encodeData(512, true); ASSERT_TRUE(response->waitForEndStream()); @@ -124,6 +137,9 @@ TEST_P(IdleTimeoutIntegrationTest, TimeoutBasic) { // Do not send any requests and validate if idle time out kicks in. ASSERT_TRUE(fake_upstream_connection_->waitForDisconnect()); test_server_->waitForCounterGe("cluster.cluster_0.upstream_cx_idle_timeout", 1); + + // Validate that the pool is deleted when it becomes idle. + test_server_->waitForGaugeEq("cluster.cluster_0.circuit_breakers.default.cx_pool_open", 0); } // Tests idle timeout behaviour with multiple requests and validates that idle timer kicks in diff --git a/test/mocks/http/conn_pool.cc b/test/mocks/http/conn_pool.cc index 035f566a6b130..7c1ffa928c0fc 100644 --- a/test/mocks/http/conn_pool.cc +++ b/test/mocks/http/conn_pool.cc @@ -1,5 +1,8 @@ #include "test/mocks/http/conn_pool.h" +using testing::_; +using testing::SaveArg; + namespace Envoy { namespace Http { namespace ConnectionPool { @@ -7,6 +10,7 @@ namespace ConnectionPool { MockInstance::MockInstance() : host_{std::make_shared>()} { ON_CALL(*this, host()).WillByDefault(Return(host_)); + ON_CALL(*this, addIdleCallback(_)).WillByDefault(SaveArg<0>(&idle_cb_)); } MockInstance::~MockInstance() = default; diff --git a/test/mocks/http/conn_pool.h b/test/mocks/http/conn_pool.h index 2f3d5b9352de2..c1f55c2f92ef8 100644 --- a/test/mocks/http/conn_pool.h +++ b/test/mocks/http/conn_pool.h @@ -29,7 +29,9 @@ class MockInstance : public Instance { // Http::ConnectionPool::Instance MOCK_METHOD(Http::Protocol, protocol, (), (const)); - MOCK_METHOD(void, addDrainedCallback, (DrainedCb cb)); + MOCK_METHOD(void, addIdleCallback, (IdleCb cb)); + MOCK_METHOD(bool, isIdle, (), (const)); + MOCK_METHOD(void, startDrain, ()); MOCK_METHOD(void, drainConnections, ()); MOCK_METHOD(bool, hasActiveConnections, (), (const)); MOCK_METHOD(Cancellable*, newStream, (ResponseDecoder & response_decoder, Callbacks& callbacks)); @@ -38,6 +40,7 @@ class MockInstance : public Instance { MOCK_METHOD(absl::string_view, protocolDescription, (), (const)); std::shared_ptr> host_; + IdleCb idle_cb_; }; } // namespace ConnectionPool diff --git a/test/mocks/tcp/mocks.cc b/test/mocks/tcp/mocks.cc index d6828f046a147..9b1a4cff79905 100644 --- a/test/mocks/tcp/mocks.cc +++ b/test/mocks/tcp/mocks.cc @@ -7,6 +7,7 @@ using testing::ReturnRef; using testing::_; using testing::Invoke; using testing::ReturnRef; +using testing::SaveArg; namespace Envoy { namespace Tcp { @@ -27,6 +28,7 @@ MockInstance::MockInstance() { return newConnectionImpl(cb); })); ON_CALL(*this, host()).WillByDefault(Return(host_)); + ON_CALL(*this, addIdleCallback(_)).WillByDefault(SaveArg<0>(&idle_cb_)); } MockInstance::~MockInstance() = default; diff --git a/test/mocks/tcp/mocks.h b/test/mocks/tcp/mocks.h index 6b486918cb99c..75e79e7aea932 100644 --- a/test/mocks/tcp/mocks.h +++ b/test/mocks/tcp/mocks.h @@ -59,7 +59,9 @@ class MockInstance : public Instance { ~MockInstance() override; // Tcp::ConnectionPool::Instance - MOCK_METHOD(void, addDrainedCallback, (DrainedCb cb)); + MOCK_METHOD(void, addIdleCallback, (IdleCb cb)); + MOCK_METHOD(bool, isIdle, (), (const)); + MOCK_METHOD(void, startDrain, ()); MOCK_METHOD(void, drainConnections, ()); MOCK_METHOD(void, closeConnections, ()); MOCK_METHOD(Cancellable*, newConnection, (Tcp::ConnectionPool::Callbacks & callbacks)); @@ -75,6 +77,7 @@ class MockInstance : public Instance { std::list> handles_; std::list callbacks_; + IdleCb idle_cb_; std::shared_ptr> host_{ new NiceMock()}; From 3de1a2d2c3cdf41928e9916310a6c981c4df1a70 Mon Sep 17 00:00:00 2001 From: Greg Greenway Date: Tue, 20 Jul 2021 12:20:05 -0700 Subject: [PATCH 02/12] default to off for now via runtime fix test failures add new integration test Signed-off-by: Greg Greenway --- source/common/runtime/runtime_features.cc | 4 +- .../upstream/cluster_manager_impl_test.cc | 4 + test/integration/BUILD | 10 +++ .../http_conn_pool_integration_test.cc | 86 +++++++++++++++++++ .../idle_timeout_integration_test.cc | 16 ---- 5 files changed, 103 insertions(+), 17 deletions(-) create mode 100644 test/integration/http_conn_pool_integration_test.cc diff --git a/source/common/runtime/runtime_features.cc b/source/common/runtime/runtime_features.cc index 74e6256751357..e351edf43794a 100644 --- a/source/common/runtime/runtime_features.cc +++ b/source/common/runtime/runtime_features.cc @@ -60,7 +60,6 @@ constexpr const char* runtime_features[] = { "envoy.reloadable_features.allow_response_for_timeout", "envoy.reloadable_features.check_unsupported_typed_per_filter_config", "envoy.reloadable_features.check_ocsp_policy", - "envoy.reloadable_features.conn_pool_delete_when_idle", "envoy.reloadable_features.dont_add_content_length_for_bodiless_requests", "envoy.reloadable_features.enable_compression_without_content_length_header", "envoy.reloadable_features.grpc_bridge_stats_disabled", @@ -109,6 +108,9 @@ constexpr const char* runtime_features[] = { constexpr const char* disabled_runtime_features[] = { // v2 is fatal-by-default. "envoy.test_only.broken_in_production.enable_deprecated_v2_api", + // Defaulting to off due to high risk. + // TODO(ggreenway): Move this to default-on during 1.20 release cycle. + "envoy.reloadable_features.conn_pool_delete_when_idle", // TODO(asraa) flip to true in a separate PR to enable the new JSON by default. "envoy.reloadable_features.remove_legacy_json", // Sentinel and test flag. diff --git a/test/common/upstream/cluster_manager_impl_test.cc b/test/common/upstream/cluster_manager_impl_test.cc index e6edf516d24da..4d110506483e1 100644 --- a/test/common/upstream/cluster_manager_impl_test.cc +++ b/test/common/upstream/cluster_manager_impl_test.cc @@ -4540,6 +4540,10 @@ TEST_F(ClusterManagerImplTest, ConnPoolsNotDrainedOnHostSetChange) { } TEST_F(ClusterManagerImplTest, ConnPoolsIdleDeleted) { + TestScopedRuntime scoped_runtime; + Runtime::LoaderSingleton::getExisting()->mergeValues( + {{"envoy.reloadable_features.conn_pool_delete_when_idle", "true"}}); + const std::string yaml = R"EOF( static_resources: clusters: diff --git a/test/integration/BUILD b/test/integration/BUILD index 873ee7db78b20..973a6c0bffe91 100644 --- a/test/integration/BUILD +++ b/test/integration/BUILD @@ -312,6 +312,16 @@ envoy_cc_test( ], ) +envoy_cc_test( + name = "http_conn_pool_integration_test", + srcs = ["http_conn_pool_integration_test.cc"], + deps = [ + ":http_protocol_integration_lib", + "//test/test_common:test_time_lib", + "@envoy_api//envoy/config/bootstrap/v3:pkg_cc_proto", + ], +) + envoy_cc_test( name = "http2_flood_integration_test", srcs = [ diff --git a/test/integration/http_conn_pool_integration_test.cc b/test/integration/http_conn_pool_integration_test.cc new file mode 100644 index 0000000000000..cbd2da87b1a52 --- /dev/null +++ b/test/integration/http_conn_pool_integration_test.cc @@ -0,0 +1,86 @@ +#include "envoy/config/bootstrap/v3/bootstrap.pb.h" + +#include "test/integration/http_protocol_integration.h" + +using testing::HasSubstr; + +namespace Envoy { +namespace { + +class ConnPoolIntegrationTest : public HttpProtocolIntegrationTest { +public: + void initialize() override { + config_helper_.addRuntimeOverride("envoy.reloadable_features.conn_pool_delete_when_idle", + "true"); + config_helper_.addConfigModifier([](envoy::config::bootstrap::v3::Bootstrap& bootstrap) { + // Set pool limit so that the test can use it's stats to validate that + // the pool is deleted. + envoy::config::cluster::v3::CircuitBreakers circuit_breakers; + auto* threshold = circuit_breakers.mutable_thresholds()->Add(); + threshold->mutable_max_connection_pools()->set_value(1); + bootstrap.mutable_static_resources() + ->mutable_clusters(0) + ->mutable_circuit_breakers() + ->MergeFrom(circuit_breakers); + }); + HttpProtocolIntegrationTest::initialize(); + } +}; + +INSTANTIATE_TEST_SUITE_P(Protocols, ConnPoolIntegrationTest, + testing::ValuesIn(HttpProtocolIntegrationTest::getProtocolTestParams()), + HttpProtocolIntegrationTest::protocolTestParamsToString); + +// Tests that conn pools are cleaned up after becoming idle due to a LocalClose +TEST_P(ConnPoolIntegrationTest, PoolCleanupAfterLocalClose) { + config_helper_.addConfigModifier([](envoy::config::bootstrap::v3::Bootstrap& bootstrap) { + // Make Envoy close the upstream connection after a single request. + bootstrap.mutable_static_resources() + ->mutable_clusters(0) + ->mutable_max_requests_per_connection() + ->set_value(1); + }); + + initialize(); + + codec_client_ = makeHttpConnection(lookupPort("http")); + auto response = codec_client_->makeRequestWithBody(default_request_headers_, 1024); + waitForNextUpstreamRequest(); + + // Validate that the circuit breaker config is setup as we expect. + test_server_->waitForGaugeEq("cluster.cluster_0.circuit_breakers.default.cx_pool_open", 1); + + upstream_request_->encodeHeaders(default_response_headers_, false); + upstream_request_->encodeData(512, true); + ASSERT_TRUE(response->waitForEndStream()); + + EXPECT_TRUE(upstream_request_->complete()); + EXPECT_TRUE(response->complete()); + + ASSERT_TRUE(fake_upstream_connection_->waitForDisconnect()); + + // Validate that the pool is deleted when it becomes idle. + test_server_->waitForGaugeEq("cluster.cluster_0.circuit_breakers.default.cx_pool_open", 0); +} + +// Tests that conn pools are cleaned up after becoming idle due to a RemoteClose +TEST_P(ConnPoolIntegrationTest, PoolCleanupAfterRemoteClose) { + initialize(); + + codec_client_ = makeHttpConnection(lookupPort("http")); + auto response = codec_client_->makeRequestWithBody(default_request_headers_, 1024); + waitForNextUpstreamRequest(); + + // Validate that the circuit breaker config is setup as we expect. + test_server_->waitForGaugeEq("cluster.cluster_0.circuit_breakers.default.cx_pool_open", 1); + + upstream_request_->encodeHeaders(default_response_headers_, false); + ASSERT_TRUE(fake_upstream_connection_->close()); + ASSERT_TRUE(response->waitForReset()); + + // Validate that the pool is deleted when it becomes idle. + test_server_->waitForGaugeEq("cluster.cluster_0.circuit_breakers.default.cx_pool_open", 0); +} + +} // namespace +} // namespace Envoy diff --git a/test/integration/idle_timeout_integration_test.cc b/test/integration/idle_timeout_integration_test.cc index 9ba8b0ff29ebe..9a8e0f75e1972 100644 --- a/test/integration/idle_timeout_integration_test.cc +++ b/test/integration/idle_timeout_integration_test.cc @@ -105,16 +105,6 @@ TEST_P(IdleTimeoutIntegrationTest, TimeoutBasic) { idle_time_out->set_seconds(seconds.count()); ConfigHelper::setProtocolOptions(*bootstrap.mutable_static_resources()->mutable_clusters(0), protocol_options); - - // Set pool limit so that the test can use it's stats to validate that - // the pool is deleted. - envoy::config::cluster::v3::CircuitBreakers circuit_breakers; - auto* threshold = circuit_breakers.mutable_thresholds()->Add(); - threshold->mutable_max_connection_pools()->set_value(1); - bootstrap.mutable_static_resources() - ->mutable_clusters(0) - ->mutable_circuit_breakers() - ->MergeFrom(circuit_breakers); }); initialize(); @@ -122,9 +112,6 @@ TEST_P(IdleTimeoutIntegrationTest, TimeoutBasic) { auto response = codec_client_->makeRequestWithBody(default_request_headers_, 1024); waitForNextUpstreamRequest(); - // Validate that the circuit breaker config is setup as we expect. - test_server_->waitForGaugeEq("cluster.cluster_0.circuit_breakers.default.cx_pool_open", 1); - upstream_request_->encodeHeaders(default_response_headers_, false); upstream_request_->encodeData(512, true); ASSERT_TRUE(response->waitForEndStream()); @@ -137,9 +124,6 @@ TEST_P(IdleTimeoutIntegrationTest, TimeoutBasic) { // Do not send any requests and validate if idle time out kicks in. ASSERT_TRUE(fake_upstream_connection_->waitForDisconnect()); test_server_->waitForCounterGe("cluster.cluster_0.upstream_cx_idle_timeout", 1); - - // Validate that the pool is deleted when it becomes idle. - test_server_->waitForGaugeEq("cluster.cluster_0.circuit_breakers.default.cx_pool_open", 0); } // Tests idle timeout behaviour with multiple requests and validates that idle timer kicks in From daf0d645fbd77f68ec2b61c0bda96627cadcf6bf Mon Sep 17 00:00:00 2001 From: Greg Greenway Date: Tue, 20 Jul 2021 14:51:08 -0700 Subject: [PATCH 03/12] fix test class name mismatch Signed-off-by: Greg Greenway --- test/integration/http_conn_pool_integration_test.cc | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-) diff --git a/test/integration/http_conn_pool_integration_test.cc b/test/integration/http_conn_pool_integration_test.cc index cbd2da87b1a52..e0e072e6e8d23 100644 --- a/test/integration/http_conn_pool_integration_test.cc +++ b/test/integration/http_conn_pool_integration_test.cc @@ -7,7 +7,7 @@ using testing::HasSubstr; namespace Envoy { namespace { -class ConnPoolIntegrationTest : public HttpProtocolIntegrationTest { +class HttpConnPoolIntegrationTest : public HttpProtocolIntegrationTest { public: void initialize() override { config_helper_.addRuntimeOverride("envoy.reloadable_features.conn_pool_delete_when_idle", @@ -27,12 +27,12 @@ class ConnPoolIntegrationTest : public HttpProtocolIntegrationTest { } }; -INSTANTIATE_TEST_SUITE_P(Protocols, ConnPoolIntegrationTest, +INSTANTIATE_TEST_SUITE_P(Protocols, HttpConnPoolIntegrationTest, testing::ValuesIn(HttpProtocolIntegrationTest::getProtocolTestParams()), HttpProtocolIntegrationTest::protocolTestParamsToString); // Tests that conn pools are cleaned up after becoming idle due to a LocalClose -TEST_P(ConnPoolIntegrationTest, PoolCleanupAfterLocalClose) { +TEST_P(HttpConnPoolIntegrationTest, PoolCleanupAfterLocalClose) { config_helper_.addConfigModifier([](envoy::config::bootstrap::v3::Bootstrap& bootstrap) { // Make Envoy close the upstream connection after a single request. bootstrap.mutable_static_resources() @@ -64,7 +64,7 @@ TEST_P(ConnPoolIntegrationTest, PoolCleanupAfterLocalClose) { } // Tests that conn pools are cleaned up after becoming idle due to a RemoteClose -TEST_P(ConnPoolIntegrationTest, PoolCleanupAfterRemoteClose) { +TEST_P(HttpConnPoolIntegrationTest, PoolCleanupAfterRemoteClose) { initialize(); codec_client_ = makeHttpConnection(lookupPort("http")); From 5545c95acaf6c1cec3c8fcabc50068a7843ba7cd Mon Sep 17 00:00:00 2001 From: Greg Greenway Date: Tue, 20 Jul 2021 14:53:25 -0700 Subject: [PATCH 04/12] add tcp pool cleanup integration test; cover original conn pool also Signed-off-by: Greg Greenway --- .../common/upstream/cluster_manager_impl.cc | 1 + .../tcp_conn_pool_integration_test.cc | 103 +++++++++++++++++- 2 files changed, 99 insertions(+), 5 deletions(-) diff --git a/source/common/upstream/cluster_manager_impl.cc b/source/common/upstream/cluster_manager_impl.cc index afc59e3c13e49..7398e786d83bc 100644 --- a/source/common/upstream/cluster_manager_impl.cc +++ b/source/common/upstream/cluster_manager_impl.cc @@ -1633,6 +1633,7 @@ Tcp::ConnectionPool::InstancePtr ProdClusterManagerFactory::allocateTcpConnPool( const Network::ConnectionSocket::OptionsSharedPtr& options, Network::TransportSocketOptionsConstSharedPtr transport_socket_options, ClusterConnectivityState& state) { + ENVOY_LOG_MISC(debug, "Allocating TCP conn pool"); if (Runtime::runtimeFeatureEnabled("envoy.reloadable_features.new_tcp_connection_pool")) { return std::make_unique(dispatcher, host, priority, options, transport_socket_options, state); diff --git a/test/integration/tcp_conn_pool_integration_test.cc b/test/integration/tcp_conn_pool_integration_test.cc index faea32638e77b..aa4bf99cdb60a 100644 --- a/test/integration/tcp_conn_pool_integration_test.cc +++ b/test/integration/tcp_conn_pool_integration_test.cc @@ -109,11 +109,34 @@ class TestFilterConfigFactory : public Server::Configuration::NamedNetworkFilter } // namespace -class TcpConnPoolIntegrationTest : public testing::TestWithParam, +struct TcpConnPoolIntegrationTestParams { + Network::Address::IpVersion version; + bool test_original_version; +}; + +std::vector getProtocolTestParams() { + std::vector ret; + + for (auto ip_version : TestEnvironment::getIpVersionsForTest()) { + ret.push_back(TcpConnPoolIntegrationTestParams{ip_version, true}); + ret.push_back(TcpConnPoolIntegrationTestParams{ip_version, false}); + } + return ret; +} + +std::string protocolTestParamsToString( + const ::testing::TestParamInfo& params) { + return absl::StrCat( + (params.param.version == Network::Address::IpVersion::v4 ? "IPv4_" : "IPv6_"), + (params.param.test_original_version == true ? "OriginalConnPool" : "NewConnPool")); +} + +class TcpConnPoolIntegrationTest : public testing::TestWithParam, public BaseIntegrationTest { public: TcpConnPoolIntegrationTest() - : BaseIntegrationTest(GetParam(), tcp_conn_pool_config), filter_resolver_(config_factory_) {} + : BaseIntegrationTest(GetParam().version, tcp_conn_pool_config), + filter_resolver_(config_factory_) {} // Called once by the gtest framework before any tests are run. static void SetUpTestSuite() { // NOLINT(readability-identifier-naming) @@ -126,7 +149,15 @@ class TcpConnPoolIntegrationTest : public testing::TestWithParamclose(); } +TEST_P(TcpConnPoolIntegrationTest, PoolCleanupEnabled) { + config_helper_.addRuntimeOverride("envoy.reloadable_features.conn_pool_delete_when_idle", "true"); + initialize(); + + std::string request1("request1"); + std::string request2("request2"); + std::string request3("request3"); + std::string response1("response1"); + std::string response2("response2"); + std::string response3("response3"); + + IntegrationTcpClientPtr tcp_client = makeTcpConnection(lookupPort("listener_0")); + + // The two requests were concurrent, so the pool was not idle, so only 1 pool has been created. + EXPECT_LOG_CONTAINS("debug", "Allocating TCP conn pool", { + // Send request 1. + ASSERT_TRUE(tcp_client->write(request1)); + FakeRawConnectionPtr fake_upstream_connection1; + ASSERT_TRUE(fake_upstreams_[0]->waitForRawConnection(fake_upstream_connection1)); + std::string data; + ASSERT_TRUE(fake_upstream_connection1->waitForData(request1.size(), &data)); + EXPECT_EQ(request1, data); + + // Send request 2. + ASSERT_TRUE(tcp_client->write(request2)); + FakeRawConnectionPtr fake_upstream_connection2; + ASSERT_TRUE(fake_upstreams_[0]->waitForRawConnection(fake_upstream_connection2)); + ASSERT_TRUE(fake_upstream_connection2->waitForData(request2.size(), &data)); + EXPECT_EQ(request2, data); + + // Send response 2. + ASSERT_TRUE(fake_upstream_connection2->write(response2)); + ASSERT_TRUE(fake_upstream_connection2->close()); + tcp_client->waitForData(response2); + + // Send response 1. + ASSERT_TRUE(fake_upstream_connection1->write(response1)); + ASSERT_TRUE(fake_upstream_connection1->close()); + tcp_client->waitForData(response1, false); + }); + + // After both requests were completed, the pool went idle and was cleaned up. Request 3 causes a + // new pool to be created. Seeing a new pool created is a proxy for directly observing that an old + // pool was cleaned up. + // TODO(ggreenway): if pool circuit breakers are implemented for tcp pools, verify cleanup by + // looking at stats such as `cluster.cluster_0.circuit_breakers.default.cx_pool_open`. + EXPECT_LOG_CONTAINS("debug", "Allocating TCP conn pool", { + // Send request 3. + ASSERT_TRUE(tcp_client->write(request3)); + FakeRawConnectionPtr fake_upstream_connection3; + ASSERT_TRUE(fake_upstreams_[0]->waitForRawConnection(fake_upstream_connection3)); + std::string data; + ASSERT_TRUE(fake_upstream_connection3->waitForData(request3.size(), &data)); + EXPECT_EQ(request3, data); + }); + + tcp_client->close(); +} + } // namespace Envoy From 1be9e5406a860d97e2c734376f77e4795f0fe545 Mon Sep 17 00:00:00 2001 From: Greg Greenway Date: Tue, 20 Jul 2021 15:54:59 -0700 Subject: [PATCH 05/12] remove assertion that was flaky on windows; it wasn't needed Signed-off-by: Greg Greenway --- test/integration/http_conn_pool_integration_test.cc | 1 - 1 file changed, 1 deletion(-) diff --git a/test/integration/http_conn_pool_integration_test.cc b/test/integration/http_conn_pool_integration_test.cc index e0e072e6e8d23..1c6e2d311aca5 100644 --- a/test/integration/http_conn_pool_integration_test.cc +++ b/test/integration/http_conn_pool_integration_test.cc @@ -76,7 +76,6 @@ TEST_P(HttpConnPoolIntegrationTest, PoolCleanupAfterRemoteClose) { upstream_request_->encodeHeaders(default_response_headers_, false); ASSERT_TRUE(fake_upstream_connection_->close()); - ASSERT_TRUE(response->waitForReset()); // Validate that the pool is deleted when it becomes idle. test_server_->waitForGaugeEq("cluster.cluster_0.circuit_breakers.default.cx_pool_open", 0); From 42f7229408f6948c00e299bbc4bf67d5421b79d1 Mon Sep 17 00:00:00 2001 From: Greg Greenway Date: Tue, 20 Jul 2021 16:52:04 -0700 Subject: [PATCH 06/12] another attempt to fix test Signed-off-by: Greg Greenway --- test/integration/http_conn_pool_integration_test.cc | 6 ++++++ 1 file changed, 6 insertions(+) diff --git a/test/integration/http_conn_pool_integration_test.cc b/test/integration/http_conn_pool_integration_test.cc index 1c6e2d311aca5..b789ec29afedf 100644 --- a/test/integration/http_conn_pool_integration_test.cc +++ b/test/integration/http_conn_pool_integration_test.cc @@ -75,6 +75,12 @@ TEST_P(HttpConnPoolIntegrationTest, PoolCleanupAfterRemoteClose) { test_server_->waitForGaugeEq("cluster.cluster_0.circuit_breakers.default.cx_pool_open", 1); upstream_request_->encodeHeaders(default_response_headers_, false); + upstream_request_->encodeData(512, true); + ASSERT_TRUE(response->waitForEndStream()); + + EXPECT_TRUE(upstream_request_->complete()); + EXPECT_TRUE(response->complete()); + ASSERT_TRUE(fake_upstream_connection_->close()); // Validate that the pool is deleted when it becomes idle. From 40b671812315392d88744f7b8ec2d11db45f2d8a Mon Sep 17 00:00:00 2001 From: Greg Greenway Date: Wed, 21 Jul 2021 09:13:10 -0700 Subject: [PATCH 07/12] clang-tidy Signed-off-by: Greg Greenway --- test/integration/http_conn_pool_integration_test.cc | 2 -- 1 file changed, 2 deletions(-) diff --git a/test/integration/http_conn_pool_integration_test.cc b/test/integration/http_conn_pool_integration_test.cc index b789ec29afedf..c98177b27de1d 100644 --- a/test/integration/http_conn_pool_integration_test.cc +++ b/test/integration/http_conn_pool_integration_test.cc @@ -2,8 +2,6 @@ #include "test/integration/http_protocol_integration.h" -using testing::HasSubstr; - namespace Envoy { namespace { From 60b7f0e1493859c570250422823c82945b3de7ed Mon Sep 17 00:00:00 2001 From: Greg Greenway Date: Wed, 21 Jul 2021 09:21:05 -0700 Subject: [PATCH 08/12] don't use deprecated config in test Signed-off-by: Greg Greenway --- test/integration/http_conn_pool_integration_test.cc | 6 ++++-- 1 file changed, 4 insertions(+), 2 deletions(-) diff --git a/test/integration/http_conn_pool_integration_test.cc b/test/integration/http_conn_pool_integration_test.cc index c98177b27de1d..a1dd4e9fb4007 100644 --- a/test/integration/http_conn_pool_integration_test.cc +++ b/test/integration/http_conn_pool_integration_test.cc @@ -33,10 +33,12 @@ INSTANTIATE_TEST_SUITE_P(Protocols, HttpConnPoolIntegrationTest, TEST_P(HttpConnPoolIntegrationTest, PoolCleanupAfterLocalClose) { config_helper_.addConfigModifier([](envoy::config::bootstrap::v3::Bootstrap& bootstrap) { // Make Envoy close the upstream connection after a single request. - bootstrap.mutable_static_resources() - ->mutable_clusters(0) + ConfigHelper::HttpProtocolOptions protocol_options; + protocol_options.mutable_common_http_protocol_options() ->mutable_max_requests_per_connection() ->set_value(1); + ConfigHelper::setProtocolOptions(*bootstrap.mutable_static_resources()->mutable_clusters(0), + protocol_options); }); initialize(); From 7158cc0b6ad00a018ce7bf22ff04ef5e8051b29e Mon Sep 17 00:00:00 2001 From: Greg Greenway Date: Wed, 21 Jul 2021 14:53:08 -0700 Subject: [PATCH 09/12] add EXPECT_LOG_CONTAINS_N_TIMES macro; fix locking in test log utility Signed-off-by: Greg Greenway --- test/test_common/logging.h | 51 +++++++++++++++++++++++++++----------- 1 file changed, 37 insertions(+), 14 deletions(-) diff --git a/test/test_common/logging.h b/test/test_common/logging.h index 124f553fe7666..2d6252b2c68ec 100644 --- a/test/test_common/logging.h +++ b/test/test_common/logging.h @@ -57,10 +57,14 @@ class LogRecordingSink : public Logger::SinkDelegate { void log(absl::string_view msg) override; void flush() override; - const std::vector& messages() const { return messages_; } + const std::vector messages() const { + absl::MutexLock ml(&mtx_); + std::vector copy(messages_); + return copy; + } private: - absl::Mutex mtx_; + mutable absl::Mutex mtx_; std::vector messages_ ABSL_GUARDED_BY(mtx_); }; @@ -98,24 +102,24 @@ using ExpectedLogMessages = std::vector; sink_ptr->setShouldEscape(escaped); \ Envoy::LogRecordingSink log_recorder(sink_ptr); \ stmt; \ - if (log_recorder.messages().empty()) { \ + auto messages = log_recorder.messages(); \ + if (messages.empty()) { \ FAIL() << "Expected message(s), but NONE was recorded."; \ } \ Envoy::ExpectedLogMessages failed_expectations; \ for (const Envoy::StringPair& expected : expected_messages) { \ const auto log_message = \ - std::find_if(log_recorder.messages().begin(), log_recorder.messages().end(), \ - [&expected](const std::string& message) { \ - return (message.find(expected.second) != std::string::npos) && \ - (message.find(expected.first) != std::string::npos); \ - }); \ - if (log_message == log_recorder.messages().end()) { \ + std::find_if(messages.begin(), messages.end(), [&expected](const std::string& message) { \ + return (message.find(expected.second) != std::string::npos) && \ + (message.find(expected.first) != std::string::npos); \ + }); \ + if (log_message == messages.end()) { \ failed_expectations.push_back(expected); \ } \ } \ if (!failed_expectations.empty()) { \ std::string failed_message; \ - absl::StrAppend(&failed_message, "\nLogs:\n ", absl::StrJoin(log_recorder.messages(), " "), \ + absl::StrAppend(&failed_message, "\nLogs:\n ", absl::StrJoin(messages, " "), \ "\n Do NOT contain:\n"); \ for (const auto& expectation : failed_expectations) { \ absl::StrAppend(&failed_message, " '", expectation.first, "', '", expectation.second, \ @@ -138,11 +142,12 @@ using ExpectedLogMessages = std::vector; Envoy::LogLevelSetter save_levels(spdlog::level::trace); \ Envoy::LogRecordingSink log_recorder(Envoy::Logger::Registry::getSink()); \ stmt; \ - for (const std::string& message : log_recorder.messages()) { \ + auto messages = log_recorder.messages(); \ + for (const std::string& message : messages) { \ if ((message.find(substr) != std::string::npos) && \ (message.find(loglevel) != std::string::npos)) { \ - FAIL() << "\nLogs:\n " << absl::StrJoin(log_recorder.messages(), " ") \ - << "\n Should NOT contain:\n '" << loglevel << "', '" << substr "'\n"; \ + FAIL() << "\nLogs:\n " << absl::StrJoin(messages, " ") << "\n Should NOT contain:\n '" \ + << loglevel << "', '" << substr "'\n"; \ } \ } \ } while (false) @@ -161,6 +166,24 @@ using ExpectedLogMessages = std::vector; EXPECT_LOG_CONTAINS_ALL_OF(message, stmt); \ } while (false) +// Validates that when stmt is executed, the supplied substring occurs exactly the specified +// number of times. +#define EXPECT_LOG_CONTAINS_N_TIMES(loglevel, substr, expected_occurrences, stmt) \ + do { \ + Envoy::LogLevelSetter save_levels(spdlog::level::trace); \ + Envoy::LogRecordingSink log_recorder(Envoy::Logger::Registry::getSink()); \ + stmt; \ + auto messages = log_recorder.messages(); \ + uint64_t actual_occurrences = 0; \ + for (const std::string& message : messages) { \ + if ((message.find(substr) != std::string::npos) && \ + (message.find(loglevel) != std::string::npos)) { \ + actual_occurrences++; \ + } \ + } \ + EXPECT_EQ(expected_occurrences, actual_occurrences); \ + } while (false) + // Validates that when stmt is executed, no logs will be emitted. // Expected equality of these values: // 0 @@ -174,7 +197,7 @@ using ExpectedLogMessages = std::vector; Envoy::LogLevelSetter save_levels(spdlog::level::trace); \ Envoy::LogRecordingSink log_recorder(Envoy::Logger::Registry::getSink()); \ stmt; \ - const std::vector& logs = log_recorder.messages(); \ + const std::vector logs = log_recorder.messages(); \ ASSERT_EQ(0, logs.size()) << " Logs:\n " << absl::StrJoin(logs, " "); \ } while (false) From 66876031113a510490aa6f8bea6fda3b0fb67e4d Mon Sep 17 00:00:00 2001 From: Greg Greenway Date: Wed, 21 Jul 2021 14:53:49 -0700 Subject: [PATCH 10/12] Fix test so that logging changes happen before server is started to avoid tsan issues. Signed-off-by: Greg Greenway --- .../tcp_conn_pool_integration_test.cc | 59 +++++++++++-------- 1 file changed, 36 insertions(+), 23 deletions(-) diff --git a/test/integration/tcp_conn_pool_integration_test.cc b/test/integration/tcp_conn_pool_integration_test.cc index aa4bf99cdb60a..e9be439f2f5d4 100644 --- a/test/integration/tcp_conn_pool_integration_test.cc +++ b/test/integration/tcp_conn_pool_integration_test.cc @@ -222,20 +222,24 @@ TEST_P(TcpConnPoolIntegrationTest, MultipleRequests) { } TEST_P(TcpConnPoolIntegrationTest, PoolCleanupEnabled) { - config_helper_.addRuntimeOverride("envoy.reloadable_features.conn_pool_delete_when_idle", "true"); - initialize(); + // The test first does two requests concurrently, resulting in a single pool (it is never idle + // between the first two), followed by going idle, then another request, which should create a + // second pool, which is why the log message is expected 2 times. If the initial pool was not + // cleaned up, only 1 pool would be created. + EXPECT_LOG_CONTAINS_N_TIMES("debug", "Allocating TCP conn pool", 2, { + config_helper_.addRuntimeOverride("envoy.reloadable_features.conn_pool_delete_when_idle", + "true"); + initialize(); + + std::string request1("request1"); + std::string request2("request2"); + std::string request3("request3"); + std::string response1("response1"); + std::string response2("response2"); + std::string response3("response3"); + + IntegrationTcpClientPtr tcp_client = makeTcpConnection(lookupPort("listener_0")); - std::string request1("request1"); - std::string request2("request2"); - std::string request3("request3"); - std::string response1("response1"); - std::string response2("response2"); - std::string response3("response3"); - - IntegrationTcpClientPtr tcp_client = makeTcpConnection(lookupPort("listener_0")); - - // The two requests were concurrent, so the pool was not idle, so only 1 pool has been created. - EXPECT_LOG_CONTAINS("debug", "Allocating TCP conn pool", { // Send request 1. ASSERT_TRUE(tcp_client->write(request1)); FakeRawConnectionPtr fake_upstream_connection1; @@ -251,6 +255,8 @@ TEST_P(TcpConnPoolIntegrationTest, PoolCleanupEnabled) { ASSERT_TRUE(fake_upstream_connection2->waitForData(request2.size(), &data)); EXPECT_EQ(request2, data); + test_server_->waitForGaugeEq("cluster.cluster_0.upstream_cx_active", 2); + // Send response 2. ASSERT_TRUE(fake_upstream_connection2->write(response2)); ASSERT_TRUE(fake_upstream_connection2->close()); @@ -260,24 +266,31 @@ TEST_P(TcpConnPoolIntegrationTest, PoolCleanupEnabled) { ASSERT_TRUE(fake_upstream_connection1->write(response1)); ASSERT_TRUE(fake_upstream_connection1->close()); tcp_client->waitForData(response1, false); - }); + test_server_->waitForGaugeEq("cluster.cluster_0.upstream_cx_active", 0); + + // After both requests were completed, the pool went idle and was cleaned up. Request 3 causes a + // new pool to be created. Seeing a new pool created is a proxy for directly observing that an + // old pool was cleaned up. + // + // TODO(ggreenway): if pool circuit breakers are implemented for tcp pools, verify cleanup by + // looking at stats such as `cluster.cluster_0.circuit_breakers.default.cx_pool_open`. - // After both requests were completed, the pool went idle and was cleaned up. Request 3 causes a - // new pool to be created. Seeing a new pool created is a proxy for directly observing that an old - // pool was cleaned up. - // TODO(ggreenway): if pool circuit breakers are implemented for tcp pools, verify cleanup by - // looking at stats such as `cluster.cluster_0.circuit_breakers.default.cx_pool_open`. - EXPECT_LOG_CONTAINS("debug", "Allocating TCP conn pool", { // Send request 3. ASSERT_TRUE(tcp_client->write(request3)); FakeRawConnectionPtr fake_upstream_connection3; ASSERT_TRUE(fake_upstreams_[0]->waitForRawConnection(fake_upstream_connection3)); - std::string data; + test_server_->waitForGaugeEq("cluster.cluster_0.upstream_cx_active", 1); ASSERT_TRUE(fake_upstream_connection3->waitForData(request3.size(), &data)); EXPECT_EQ(request3, data); - }); - tcp_client->close(); + ASSERT_TRUE(fake_upstream_connection3->write(response3)); + ASSERT_TRUE(fake_upstream_connection3->close()); + tcp_client->waitForData(response3, false); + + test_server_->waitForGaugeEq("cluster.cluster_0.upstream_cx_active", 0); + + tcp_client->close(); + }); } } // namespace Envoy From 0951e0a70b0b3680dbf87f3df12369dc0291cde9 Mon Sep 17 00:00:00 2001 From: Greg Greenway Date: Thu, 22 Jul 2021 10:00:40 -0700 Subject: [PATCH 11/12] add tcp conn pool shutdown test Signed-off-by: Greg Greenway --- .../tcp_conn_pool_integration_test.cc | 18 ++++++++++++++++++ 1 file changed, 18 insertions(+) diff --git a/test/integration/tcp_conn_pool_integration_test.cc b/test/integration/tcp_conn_pool_integration_test.cc index e9be439f2f5d4..54245a2075929 100644 --- a/test/integration/tcp_conn_pool_integration_test.cc +++ b/test/integration/tcp_conn_pool_integration_test.cc @@ -293,4 +293,22 @@ TEST_P(TcpConnPoolIntegrationTest, PoolCleanupEnabled) { }); } +TEST_P(TcpConnPoolIntegrationTest, ShutdownWithOpenConnections) { + initialize(); + + IntegrationTcpClientPtr tcp_client = makeTcpConnection(lookupPort("listener_0")); + + // Establish downstream and upstream connections. + ASSERT_TRUE(tcp_client->write("hello")); + FakeRawConnectionPtr fake_upstream_connection; + ASSERT_TRUE(fake_upstreams_[0]->waitForRawConnection(fake_upstream_connection)); + ASSERT_TRUE(fake_upstream_connection->waitForData(5)); + + test_server_.reset(); + ASSERT_TRUE(fake_upstream_connection->waitForDisconnect()); + tcp_client->waitForDisconnect(); + + // Success criteria is that no ASSERTs fire and there are no leaks. +} + } // namespace Envoy From 4e871d744422d12db7fb0092ed7dd22534c74976 Mon Sep 17 00:00:00 2001 From: Greg Greenway Date: Fri, 23 Jul 2021 08:57:38 -0700 Subject: [PATCH 12/12] re-alphabetize release note after misordered merge Signed-off-by: Greg Greenway --- docs/root/version_history/current.rst | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/docs/root/version_history/current.rst b/docs/root/version_history/current.rst index eb539742f1dd5..35001aacd318f 100644 --- a/docs/root/version_history/current.rst +++ b/docs/root/version_history/current.rst @@ -36,8 +36,8 @@ Bug Fixes --------- *Changes expected to improve the state of the world and are unlikely to have negative effects* -* cluster: delete pools when they're idle to fix unbounded memory use when using PROXY protocol upstream with tcp_proxy. This behavior can be temporarily reverted by setting the ``envoy.reloadable_features.conn_pool_delete_when_idle`` runtime guard to false. * access log: fix `%UPSTREAM_CLUSTER%` when used in http upstream access logs. Previously, it was always logging as an unset value. +* cluster: delete pools when they're idle to fix unbounded memory use when using PROXY protocol upstream with tcp_proxy. This behavior can be temporarily reverted by setting the ``envoy.reloadable_features.conn_pool_delete_when_idle`` runtime guard to false. * xray: fix the AWS X-Ray tracer bug where span's error, fault and throttle information was not reported properly as per the `AWS X-Ray documentation `_. Before this fix, server error was reported under 'annotations' section of the segment data. Removed Config or Runtime