From b474d23d145fdfb28d9b5b2520022f8b28568128 Mon Sep 17 00:00:00 2001 From: Yuchen Dai Date: Thu, 12 Nov 2020 21:29:34 +0000 Subject: [PATCH 01/40] destroy hosts on master Signed-off-by: Yuchen Dai --- .../common/upstream/cluster_manager_impl.cc | 25 +++++++++++++------ 1 file changed, 17 insertions(+), 8 deletions(-) diff --git a/source/common/upstream/cluster_manager_impl.cc b/source/common/upstream/cluster_manager_impl.cc index 6eaef38a901b..5b8918f22d84 100644 --- a/source/common/upstream/cluster_manager_impl.cc +++ b/source/common/upstream/cluster_manager_impl.cc @@ -8,6 +8,7 @@ #include #include +#include "common/common/macros.h" #include "envoy/admin/v3/config_dump.pb.h" #include "envoy/config/bootstrap/v3/bootstrap.pb.h" #include "envoy/config/cluster/v3/cluster.pb.h" @@ -931,14 +932,22 @@ void ClusterManagerImpl::postThreadLocalClusterUpdate(const Cluster& cluster, ui const HostVector& hosts_removed) { const auto& host_set = cluster.prioritySet().hostSetsPerPriority()[priority]; - tls_.runOnAllThreads([name = cluster.info()->name(), priority, - update_params = HostSetImpl::updateHostsParams(*host_set), - locality_weights = host_set->localityWeights(), hosts_added, hosts_removed, - overprovisioning_factor = host_set->overprovisioningFactor()]( - OptRef cluster_manager) { - cluster_manager->updateClusterMembership(name, priority, update_params, locality_weights, - hosts_added, hosts_removed, overprovisioning_factor); - }); + tls_.runOnAllThreads( + [name = cluster.info()->name(), priority, + update_params = HostSetImpl::updateHostsParams(*host_set), + locality_weights = host_set->localityWeights(), hosts_added, hosts_removed, + overprovisioning_factor = host_set->overprovisioningFactor()]( + OptRef cluster_manager) { + cluster_manager->updateClusterMembership(name, priority, update_params, locality_weights, + hosts_added, hosts_removed, + overprovisioning_factor); + }, + // This completion is guaranteed to be executed on master thread. + // Still the host can be snapped by dataplane and need to addressed separately. + [hosts_added_guard = hosts_added, hosts_removed_guard = hosts_removed]() { + UNREFERENCED_PARAMETER(hosts_added_guard); + UNREFERENCED_PARAMETER(hosts_removed_guard); + }); } void ClusterManagerImpl::postThreadLocalHealthFailure(const HostSharedPtr& host) { From 7b933b1c432993b55953838b609ecc32d8874c3c Mon Sep 17 00:00:00 2001 From: Yuchen Dai Date: Sun, 15 Nov 2020 06:41:52 +0000 Subject: [PATCH 02/40] tobetrypost Signed-off-by: Yuchen Dai --- include/envoy/event/dispatcher.h | 2 +- source/common/event/dispatcher_impl.cc | 24 +++++++++++++++++-- source/common/event/dispatcher_impl.h | 4 ++-- source/common/upstream/BUILD | 1 + source/common/upstream/upstream_impl.cc | 19 ++++++++++++--- test/common/http/http1/conn_pool_test.cc | 2 +- .../upstream/cluster_manager_impl_test.cc | 2 ++ test/mocks/event/mocks.cc | 2 +- test/mocks/event/mocks.h | 2 +- 9 files changed, 47 insertions(+), 11 deletions(-) diff --git a/include/envoy/event/dispatcher.h b/include/envoy/event/dispatcher.h index bd4e9512aa4b..4db13426f473 100644 --- a/include/envoy/event/dispatcher.h +++ b/include/envoy/event/dispatcher.h @@ -211,7 +211,7 @@ class Dispatcher { * Posts a functor to the dispatcher. This is safe cross thread. The functor runs in the context * of the dispatcher event loop which may be on a different thread than the caller. */ - virtual void post(PostCb callback) PURE; + virtual bool post(PostCb callback) PURE; /** * Runs the event loop. This will not return until exit() is called either from within a callback diff --git a/source/common/event/dispatcher_impl.cc b/source/common/event/dispatcher_impl.cc index d615e5986f5d..12c6e513021b 100644 --- a/source/common/event/dispatcher_impl.cc +++ b/source/common/event/dispatcher_impl.cc @@ -216,17 +216,34 @@ void DispatcherImpl::deferredDelete(DeferredDeletablePtr&& to_delete) { } } -void DispatcherImpl::exit() { base_scheduler_.loopExit(); } +void DispatcherImpl::exit() { + { + Thread::LockGuard lock(post_lock_); + exited_ = true; + // No more post! + } + base_scheduler_.loopExit(); + { + FANCY_LOG(debug, "lambdai: running postcallbacks after exit"); + // TODO(lambdai): runtime key. + runPostCallbacks(); + FANCY_LOG(debug, "lambdai: complete postcallbacks after exit"); + } +} SignalEventPtr DispatcherImpl::listenForSignal(int signal_num, SignalCb cb) { ASSERT(isThreadSafe()); return SignalEventPtr{new SignalEventImpl(*this, signal_num, cb)}; } -void DispatcherImpl::post(std::function callback) { +bool DispatcherImpl::post(std::function callback) { bool do_post; { Thread::LockGuard lock(post_lock_); + // TODO(lambdai): For compatibility we should allow blind post. + if (exited_) { + return false; + } do_post = post_callbacks_.empty(); post_callbacks_.push_back(callback); } @@ -234,6 +251,7 @@ void DispatcherImpl::post(std::function callback) { if (do_post) { post_cb_->scheduleCallbackCurrentIteration(); } + return true; } void DispatcherImpl::run(RunType type) { @@ -244,7 +262,9 @@ void DispatcherImpl::run(RunType type) { // not guarantee that events are run in any particular order. So even if we post() and call // event_base_once() before some other event, the other event might get called first. runPostCallbacks(); + exited_ = false; base_scheduler_.run(type); + exited_ = true; } MonotonicTime DispatcherImpl::approximateMonotonicTime() const { diff --git a/source/common/event/dispatcher_impl.h b/source/common/event/dispatcher_impl.h index 62c10920d7fe..c657d03ac934 100644 --- a/source/common/event/dispatcher_impl.h +++ b/source/common/event/dispatcher_impl.h @@ -72,7 +72,7 @@ class DispatcherImpl : Logger::Loggable, void deferredDelete(DeferredDeletablePtr&& to_delete) override; void exit() override; SignalEventPtr listenForSignal(int signal_num, SignalCb cb) override; - void post(std::function callback) override; + bool post(std::function callback) override; void run(RunType type) override; Buffer::WatermarkFactory& getWatermarkFactory() override { return *buffer_factory_; } const ScopeTrackedObject* setTrackedObject(const ScopeTrackedObject* object) override { @@ -132,7 +132,7 @@ class DispatcherImpl : Logger::Loggable, bool isThreadSafe() const override { return run_tid_.isEmpty() || run_tid_ == api_.threadFactory().currentThreadId(); } - + std::atomic exited_{}; const std::string name_; Api::Api& api_; std::string stats_prefix_; diff --git a/source/common/upstream/BUILD b/source/common/upstream/BUILD index 9219e59d768d..3426a9acd213 100644 --- a/source/common/upstream/BUILD +++ b/source/common/upstream/BUILD @@ -433,6 +433,7 @@ envoy_cc_library( ":strict_dns_cluster_lib", ":upstream_includes", ":transport_socket_match_lib", + "//source/common/event:deferred_task", "@envoy_api//envoy/config/cluster/v3:pkg_cc_proto", "@envoy_api//envoy/config/core/v3:pkg_cc_proto", "@envoy_api//envoy/config/endpoint/v3:pkg_cc_proto", diff --git a/source/common/upstream/upstream_impl.cc b/source/common/upstream/upstream_impl.cc index 8e6a6db3c507..2c30e56d1947 100644 --- a/source/common/upstream/upstream_impl.cc +++ b/source/common/upstream/upstream_impl.cc @@ -28,6 +28,7 @@ #include "envoy/upstream/upstream.h" #include "common/common/enum_to_int.h" +#include "common/event/deferred_task.h" #include "common/common/fmt.h" #include "common/common/utility.h" #include "common/config/utility.h" @@ -900,9 +901,21 @@ ClusterImplBase::ClusterImplBase( auto socket_factory = createTransportSocketFactory(cluster, factory_context); auto socket_matcher = std::make_unique( cluster.transport_socket_matches(), factory_context, socket_factory, *stats_scope); - info_ = std::make_unique(cluster, factory_context.clusterManager().bindConfig(), - runtime, std::move(socket_matcher), - std::move(stats_scope), added_via_api, factory_context); + auto& dispatcher = factory_context.dispatcher(); + info_ = std::shared_ptr( + new ClusterInfoImpl(cluster, factory_context.clusterManager().bindConfig(), runtime, + std::move(socket_matcher), std::move(stats_scope), added_via_api, + factory_context), + [&dispatcher](const ClusterInfoImpl* self) { + FANCY_LOG(debug, "lambdai: schedule destroy cluster info {} on this thread", self->name()); + if (!dispatcher.post([self]() { + FANCY_LOG(debug, "lambdai: execute destroy cluster info {} on this thread", self->name()); + delete self; + })) { + FANCY_LOG(debug, "lambdai: cannot post. Exited? Executing destroy cluster info {} on this thread", self->name()); + delete self; + } + }); // Create the default (empty) priority set before registering callbacks to // avoid getting an update the first time it is accessed. priority_set_.getOrCreateHostSet(0); diff --git a/test/common/http/http1/conn_pool_test.cc b/test/common/http/http1/conn_pool_test.cc index 3803add0f724..34b7aaf5615a 100644 --- a/test/common/http/http1/conn_pool_test.cc +++ b/test/common/http/http1/conn_pool_test.cc @@ -116,7 +116,7 @@ class ConnPoolImplForTest : public FixedHttpConnPoolImpl { } void expectEnableUpstreamReady() { - EXPECT_CALL(mock_dispatcher_, post(_)).WillOnce(SaveArg<0>(&post_cb_)); + EXPECT_CALL(mock_dispatcher_, post(_)).WillOnce(DoAll(SaveArg<0>(&post_cb_), Return(true))); } void expectAndRunUpstreamReady() { post_cb_(); } diff --git a/test/common/upstream/cluster_manager_impl_test.cc b/test/common/upstream/cluster_manager_impl_test.cc index 2127623a9400..40ef9f3ebe52 100644 --- a/test/common/upstream/cluster_manager_impl_test.cc +++ b/test/common/upstream/cluster_manager_impl_test.cc @@ -839,6 +839,7 @@ TEST_F(ClusterManagerImplTest, HttpHealthChecker) { .WillOnce(Return(connection)); create(parseBootstrapFromV3Yaml(yaml)); factory_.tls_.shutdownThread(); + factory_.dispatcher_.to_delete_.clear(); } TEST_F(ClusterManagerImplTest, UnknownCluster) { @@ -3966,6 +3967,7 @@ TEST_F(ClusterManagerImplTest, ConnPoolsNotDrainedOnHostSetChange) { cluster.prioritySet().updateHosts( 0, HostSetImpl::partitionHosts(hosts_ptr, HostsPerLocalityImpl::empty()), nullptr, hosts_added, {}, 100); + //cluster_manager_->removeCluster() } TEST_F(ClusterManagerImplTest, InvalidPriorityLocalClusterNameStatic) { diff --git a/test/mocks/event/mocks.cc b/test/mocks/event/mocks.cc index 215e7cafbdb4..8239945a1b42 100644 --- a/test/mocks/event/mocks.cc +++ b/test/mocks/event/mocks.cc @@ -24,7 +24,7 @@ MockDispatcher::MockDispatcher(const std::string& name) : name_(name) { to_delete_.clear(); })); ON_CALL(*this, createTimer_(_)).WillByDefault(ReturnNew>()); - ON_CALL(*this, post(_)).WillByDefault(Invoke([](PostCb cb) -> void { cb(); })); + ON_CALL(*this, post(_)).WillByDefault(Invoke([](PostCb cb) -> bool { cb(); return true;})); } MockDispatcher::~MockDispatcher() = default; diff --git a/test/mocks/event/mocks.h b/test/mocks/event/mocks.h index 861f0a10e340..aa031e0a415b 100644 --- a/test/mocks/event/mocks.h +++ b/test/mocks/event/mocks.h @@ -128,7 +128,7 @@ class MockDispatcher : public Dispatcher { MOCK_METHOD(void, deferredDelete_, (DeferredDeletable * to_delete)); MOCK_METHOD(void, exit, ()); MOCK_METHOD(SignalEvent*, listenForSignal_, (int signal_num, SignalCb cb)); - MOCK_METHOD(void, post, (std::function callback)); + MOCK_METHOD(bool, post, (std::function callback)); MOCK_METHOD(void, run, (RunType type)); MOCK_METHOD(const ScopeTrackedObject*, setTrackedObject, (const ScopeTrackedObject* object)); MOCK_METHOD(bool, isThreadSafe, (), (const)); From 65ed52e1ee50093c490581e18dcbc2361d322ae4 Mon Sep 17 00:00:00 2001 From: Yuchen Dai Date: Thu, 19 Nov 2020 00:23:36 +0000 Subject: [PATCH 03/40] to tryPost Signed-off-by: Yuchen Dai --- include/envoy/event/dispatcher.h | 7 ++++- source/common/event/dispatcher_impl.cc | 30 ++++++++++++++++--- source/common/event/dispatcher_impl.h | 3 +- source/common/upstream/upstream_impl.cc | 11 ++++--- test/common/http/http1/conn_pool_test.cc | 2 +- .../upstream/cluster_manager_impl_test.cc | 1 - test/mocks/event/mocks.cc | 2 +- test/mocks/event/mocks.h | 3 +- 8 files changed, 45 insertions(+), 14 deletions(-) diff --git a/include/envoy/event/dispatcher.h b/include/envoy/event/dispatcher.h index 4db13426f473..bd58f8b77030 100644 --- a/include/envoy/event/dispatcher.h +++ b/include/envoy/event/dispatcher.h @@ -211,7 +211,12 @@ class Dispatcher { * Posts a functor to the dispatcher. This is safe cross thread. The functor runs in the context * of the dispatcher event loop which may be on a different thread than the caller. */ - virtual bool post(PostCb callback) PURE; + virtual void post(PostCb callback) PURE; + + /** + * Similar to `post()` but return false if the dispatcher rejects. + */ + virtual bool tryPost(PostCb callback) PURE; /** * Runs the event loop. This will not return until exit() is called either from within a callback diff --git a/source/common/event/dispatcher_impl.cc b/source/common/event/dispatcher_impl.cc index 12c6e513021b..b62f914815f2 100644 --- a/source/common/event/dispatcher_impl.cc +++ b/source/common/event/dispatcher_impl.cc @@ -236,7 +236,20 @@ SignalEventPtr DispatcherImpl::listenForSignal(int signal_num, SignalCb cb) { return SignalEventPtr{new SignalEventImpl(*this, signal_num, cb)}; } -bool DispatcherImpl::post(std::function callback) { +void DispatcherImpl::post(std::function callback) { + bool do_post; + { + Thread::LockGuard lock(post_lock_); + do_post = post_callbacks_.empty(); + post_callbacks_.push_back(callback); + } + + if (do_post) { + post_cb_->scheduleCallbackCurrentIteration(); + } +} + +bool DispatcherImpl::tryPost(std::function callback) { bool do_post; { Thread::LockGuard lock(post_lock_); @@ -256,15 +269,24 @@ bool DispatcherImpl::post(std::function callback) { void DispatcherImpl::run(RunType type) { run_tid_ = api_.threadFactory().currentThreadId(); - + { + // Allows tryPost. + FANCY_LOG(debug, "lambdai: run {}", type); + Thread::LockGuard lock(post_lock_); + exited_ = false; + } // Flush all post callbacks before we run the event loop. We do this because there are post // callbacks that have to get run before the initial event loop starts running. libevent does // not guarantee that events are run in any particular order. So even if we post() and call // event_base_once() before some other event, the other event might get called first. runPostCallbacks(); - exited_ = false; base_scheduler_.run(type); - exited_ = true; + { + FANCY_LOG(debug, "lambdai: run return {}", type); + // Reject all the follow up tryPost. + Thread::LockGuard lock(post_lock_); + exited_ = true; + } } MonotonicTime DispatcherImpl::approximateMonotonicTime() const { diff --git a/source/common/event/dispatcher_impl.h b/source/common/event/dispatcher_impl.h index c657d03ac934..c3477ff46ae4 100644 --- a/source/common/event/dispatcher_impl.h +++ b/source/common/event/dispatcher_impl.h @@ -72,7 +72,8 @@ class DispatcherImpl : Logger::Loggable, void deferredDelete(DeferredDeletablePtr&& to_delete) override; void exit() override; SignalEventPtr listenForSignal(int signal_num, SignalCb cb) override; - bool post(std::function callback) override; + void post(std::function callback) override; + bool tryPost(std::function callback) override; void run(RunType type) override; Buffer::WatermarkFactory& getWatermarkFactory() override { return *buffer_factory_; } const ScopeTrackedObject* setTrackedObject(const ScopeTrackedObject* object) override { diff --git a/source/common/upstream/upstream_impl.cc b/source/common/upstream/upstream_impl.cc index 2c30e56d1947..c20711999784 100644 --- a/source/common/upstream/upstream_impl.cc +++ b/source/common/upstream/upstream_impl.cc @@ -28,7 +28,6 @@ #include "envoy/upstream/upstream.h" #include "common/common/enum_to_int.h" -#include "common/event/deferred_task.h" #include "common/common/fmt.h" #include "common/common/utility.h" #include "common/config/utility.h" @@ -908,11 +907,15 @@ ClusterImplBase::ClusterImplBase( factory_context), [&dispatcher](const ClusterInfoImpl* self) { FANCY_LOG(debug, "lambdai: schedule destroy cluster info {} on this thread", self->name()); - if (!dispatcher.post([self]() { - FANCY_LOG(debug, "lambdai: execute destroy cluster info {} on this thread", self->name()); + if (!dispatcher.tryPost([self]() { + // TODO(lambdai): Yet there is risk that master dispatcher receives the function but doesn't execute during the shutdown. + // We can either + // 1) Introduce folly::function which supports with unique_ptr capture and destroy cluster info by RAII, or + // 2) Call run post callback in master thread after no worker post back. + FANCY_LOG(debug, "lambdai: execute destroy cluster info {} on this thread. Master thread is expected.", self->name()); delete self; })) { - FANCY_LOG(debug, "lambdai: cannot post. Exited? Executing destroy cluster info {} on this thread", self->name()); + FANCY_LOG(debug, "lambdai: cannot post. Has the master thread exited? Executing destroy cluster info {} on this thread.", self->name()); delete self; } }); diff --git a/test/common/http/http1/conn_pool_test.cc b/test/common/http/http1/conn_pool_test.cc index 34b7aaf5615a..3803add0f724 100644 --- a/test/common/http/http1/conn_pool_test.cc +++ b/test/common/http/http1/conn_pool_test.cc @@ -116,7 +116,7 @@ class ConnPoolImplForTest : public FixedHttpConnPoolImpl { } void expectEnableUpstreamReady() { - EXPECT_CALL(mock_dispatcher_, post(_)).WillOnce(DoAll(SaveArg<0>(&post_cb_), Return(true))); + EXPECT_CALL(mock_dispatcher_, post(_)).WillOnce(SaveArg<0>(&post_cb_)); } void expectAndRunUpstreamReady() { post_cb_(); } diff --git a/test/common/upstream/cluster_manager_impl_test.cc b/test/common/upstream/cluster_manager_impl_test.cc index 40ef9f3ebe52..01540f1d4e77 100644 --- a/test/common/upstream/cluster_manager_impl_test.cc +++ b/test/common/upstream/cluster_manager_impl_test.cc @@ -3967,7 +3967,6 @@ TEST_F(ClusterManagerImplTest, ConnPoolsNotDrainedOnHostSetChange) { cluster.prioritySet().updateHosts( 0, HostSetImpl::partitionHosts(hosts_ptr, HostsPerLocalityImpl::empty()), nullptr, hosts_added, {}, 100); - //cluster_manager_->removeCluster() } TEST_F(ClusterManagerImplTest, InvalidPriorityLocalClusterNameStatic) { diff --git a/test/mocks/event/mocks.cc b/test/mocks/event/mocks.cc index 8239945a1b42..215e7cafbdb4 100644 --- a/test/mocks/event/mocks.cc +++ b/test/mocks/event/mocks.cc @@ -24,7 +24,7 @@ MockDispatcher::MockDispatcher(const std::string& name) : name_(name) { to_delete_.clear(); })); ON_CALL(*this, createTimer_(_)).WillByDefault(ReturnNew>()); - ON_CALL(*this, post(_)).WillByDefault(Invoke([](PostCb cb) -> bool { cb(); return true;})); + ON_CALL(*this, post(_)).WillByDefault(Invoke([](PostCb cb) -> void { cb(); })); } MockDispatcher::~MockDispatcher() = default; diff --git a/test/mocks/event/mocks.h b/test/mocks/event/mocks.h index aa031e0a415b..62ca3486f63f 100644 --- a/test/mocks/event/mocks.h +++ b/test/mocks/event/mocks.h @@ -128,7 +128,8 @@ class MockDispatcher : public Dispatcher { MOCK_METHOD(void, deferredDelete_, (DeferredDeletable * to_delete)); MOCK_METHOD(void, exit, ()); MOCK_METHOD(SignalEvent*, listenForSignal_, (int signal_num, SignalCb cb)); - MOCK_METHOD(bool, post, (std::function callback)); + MOCK_METHOD(void, post, (std::function callback)); + MOCK_METHOD(bool, tryPost, (std::function callback)); MOCK_METHOD(void, run, (RunType type)); MOCK_METHOD(const ScopeTrackedObject*, setTrackedObject, (const ScopeTrackedObject* object)); MOCK_METHOD(bool, isThreadSafe, (), (const)); From df9beaae353d55866aebc233688e8856584b1b80 Mon Sep 17 00:00:00 2001 From: Yuchen Dai Date: Fri, 20 Nov 2020 18:24:53 +0000 Subject: [PATCH 04/40] revert hosts guard Signed-off-by: Yuchen Dai --- .../common/upstream/cluster_manager_impl.cc | 24 +++++++------------ 1 file changed, 8 insertions(+), 16 deletions(-) diff --git a/source/common/upstream/cluster_manager_impl.cc b/source/common/upstream/cluster_manager_impl.cc index 5b8918f22d84..04e9bc1cd889 100644 --- a/source/common/upstream/cluster_manager_impl.cc +++ b/source/common/upstream/cluster_manager_impl.cc @@ -932,22 +932,14 @@ void ClusterManagerImpl::postThreadLocalClusterUpdate(const Cluster& cluster, ui const HostVector& hosts_removed) { const auto& host_set = cluster.prioritySet().hostSetsPerPriority()[priority]; - tls_.runOnAllThreads( - [name = cluster.info()->name(), priority, - update_params = HostSetImpl::updateHostsParams(*host_set), - locality_weights = host_set->localityWeights(), hosts_added, hosts_removed, - overprovisioning_factor = host_set->overprovisioningFactor()]( - OptRef cluster_manager) { - cluster_manager->updateClusterMembership(name, priority, update_params, locality_weights, - hosts_added, hosts_removed, - overprovisioning_factor); - }, - // This completion is guaranteed to be executed on master thread. - // Still the host can be snapped by dataplane and need to addressed separately. - [hosts_added_guard = hosts_added, hosts_removed_guard = hosts_removed]() { - UNREFERENCED_PARAMETER(hosts_added_guard); - UNREFERENCED_PARAMETER(hosts_removed_guard); - }); + tls_.runOnAllThreads([name = cluster.info()->name(), priority, + update_params = HostSetImpl::updateHostsParams(*host_set), + locality_weights = host_set->localityWeights(), hosts_added, hosts_removed, + overprovisioning_factor = host_set->overprovisioningFactor()]( + OptRef cluster_manager) { + cluster_manager->updateClusterMembership(name, priority, update_params, locality_weights, + hosts_added, hosts_removed, overprovisioning_factor); + }); } void ClusterManagerImpl::postThreadLocalHealthFailure(const HostSharedPtr& host) { From b647d0c3c5aae4372e049caf6271e4972f4191bf Mon Sep 17 00:00:00 2001 From: Yuchen Dai Date: Fri, 20 Nov 2020 22:51:31 +0000 Subject: [PATCH 05/40] fix cluster test Signed-off-by: Yuchen Dai --- .../common/upstream/cluster_manager_impl.cc | 2 +- source/common/upstream/upstream_impl.cc | 22 ++++++++++++------- .../upstream/cluster_manager_impl_test.cc | 1 + test/mocks/event/wrapped_dispatcher.h | 4 ++++ 4 files changed, 20 insertions(+), 9 deletions(-) diff --git a/source/common/upstream/cluster_manager_impl.cc b/source/common/upstream/cluster_manager_impl.cc index 257fd242bc1b..a53eb57c9706 100644 --- a/source/common/upstream/cluster_manager_impl.cc +++ b/source/common/upstream/cluster_manager_impl.cc @@ -8,7 +8,6 @@ #include #include -#include "common/common/macros.h" #include "envoy/admin/v3/config_dump.pb.h" #include "envoy/config/bootstrap/v3/bootstrap.pb.h" #include "envoy/config/cluster/v3/cluster.pb.h" @@ -21,6 +20,7 @@ #include "common/common/assert.h" #include "common/common/enum_to_int.h" #include "common/common/fmt.h" +#include "common/common/macros.h" #include "common/common/utility.h" #include "common/config/new_grpc_mux_impl.h" #include "common/config/utility.h" diff --git a/source/common/upstream/upstream_impl.cc b/source/common/upstream/upstream_impl.cc index c20711999784..079ffdd34ee0 100644 --- a/source/common/upstream/upstream_impl.cc +++ b/source/common/upstream/upstream_impl.cc @@ -908,14 +908,20 @@ ClusterImplBase::ClusterImplBase( [&dispatcher](const ClusterInfoImpl* self) { FANCY_LOG(debug, "lambdai: schedule destroy cluster info {} on this thread", self->name()); if (!dispatcher.tryPost([self]() { - // TODO(lambdai): Yet there is risk that master dispatcher receives the function but doesn't execute during the shutdown. - // We can either - // 1) Introduce folly::function which supports with unique_ptr capture and destroy cluster info by RAII, or - // 2) Call run post callback in master thread after no worker post back. - FANCY_LOG(debug, "lambdai: execute destroy cluster info {} on this thread. Master thread is expected.", self->name()); - delete self; - })) { - FANCY_LOG(debug, "lambdai: cannot post. Has the master thread exited? Executing destroy cluster info {} on this thread.", self->name()); + // TODO(lambdai): Yet there is risk that master dispatcher receives the function but + // doesn't execute during the shutdown. We can either 1) Introduce folly::function + // which supports with unique_ptr capture and destroy cluster info by RAII, or 2) Call + // run post callback in master thread after no worker post back. + FANCY_LOG(debug, + "lambdai: execute destroy cluster info {} on this thread. Master thread is " + "expected.", + self->name()); + delete self; + })) { + FANCY_LOG(debug, + "lambdai: cannot post. Has the master thread exited? Executing destroy cluster " + "info {} on this thread.", + self->name()); delete self; } }); diff --git a/test/common/upstream/cluster_manager_impl_test.cc b/test/common/upstream/cluster_manager_impl_test.cc index 0d1e40dbe969..21a26ede8ecf 100644 --- a/test/common/upstream/cluster_manager_impl_test.cc +++ b/test/common/upstream/cluster_manager_impl_test.cc @@ -837,6 +837,7 @@ TEST_F(ClusterManagerImplTest, HttpHealthChecker) { createClientConnection_( PointeesEq(Network::Utility::resolveUrl("tcp://127.0.0.1:11001")), _, _, _)) .WillOnce(Return(connection)); + EXPECT_CALL(factory_.dispatcher_, tryPost(_)).Times(1); create(parseBootstrapFromV3Yaml(yaml)); factory_.tls_.shutdownThread(); factory_.dispatcher_.to_delete_.clear(); diff --git a/test/mocks/event/wrapped_dispatcher.h b/test/mocks/event/wrapped_dispatcher.h index 74e935328984..6a60168c1d0f 100644 --- a/test/mocks/event/wrapped_dispatcher.h +++ b/test/mocks/event/wrapped_dispatcher.h @@ -94,6 +94,10 @@ class WrappedDispatcher : public Dispatcher { void post(std::function callback) override { impl_.post(std::move(callback)); } + bool tryPost(std::function callback) override { + return impl_.tryPost(std::move(callback)); + } + void run(RunType type) override { impl_.run(type); } Buffer::WatermarkFactory& getWatermarkFactory() override { return impl_.getWatermarkFactory(); } From 8c6266d6ce9be76e032f76382ccad4e0a9501dfc Mon Sep 17 00:00:00 2001 From: Yuchen Dai Date: Fri, 20 Nov 2020 23:14:46 +0000 Subject: [PATCH 06/40] fix server fuzz test Signed-off-by: Yuchen Dai --- source/common/event/dispatcher_impl.cc | 10 +++++++++- 1 file changed, 9 insertions(+), 1 deletion(-) diff --git a/source/common/event/dispatcher_impl.cc b/source/common/event/dispatcher_impl.cc index 9f6d19adc83e..81d5686905e0 100644 --- a/source/common/event/dispatcher_impl.cc +++ b/source/common/event/dispatcher_impl.cc @@ -251,6 +251,10 @@ void DispatcherImpl::post(std::function callback) { bool DispatcherImpl::tryPost(std::function callback) { bool do_post; + // Post master to master. Skip. + if (isThreadSafe()) { + return false; + } { Thread::LockGuard lock(post_lock_); // TODO(lambdai): For compatibility we should allow blind post. @@ -273,7 +277,9 @@ void DispatcherImpl::run(RunType type) { // Allows tryPost. FANCY_LOG(debug, "lambdai: run {}", type); Thread::LockGuard lock(post_lock_); - exited_ = false; + if (type == RunType::Block) { + exited_ = false; + } } // Flush all post callbacks before we run the event loop. We do this because there are post // callbacks that have to get run before the initial event loop starts running. libevent does @@ -287,6 +293,8 @@ void DispatcherImpl::run(RunType type) { Thread::LockGuard lock(post_lock_); exited_ = true; } + // TODO(lambdai): reconsider this. + runPostCallbacks(); } MonotonicTime DispatcherImpl::approximateMonotonicTime() const { From 564b17a5c28c6aff1f12b79fed66e78ec5de137a Mon Sep 17 00:00:00 2001 From: Yuchen Dai Date: Fri, 5 Feb 2021 03:54:55 -0800 Subject: [PATCH 07/40] cleanup Signed-off-by: Yuchen Dai --- include/envoy/event/dispatcher.h | 2 +- source/common/event/dispatcher_impl.cc | 19 +++++-------- source/common/event/dispatcher_impl.h | 2 +- source/common/upstream/upstream_impl.cc | 28 ++++++++----------- .../upstream/logical_dns_cluster_test.cc | 2 +- .../upstream/original_dst_cluster_test.cc | 2 +- test/mocks/event/mocks.h | 2 +- test/mocks/event/wrapped_dispatcher.h | 2 +- 8 files changed, 24 insertions(+), 35 deletions(-) diff --git a/include/envoy/event/dispatcher.h b/include/envoy/event/dispatcher.h index 44615267dab6..ea6a17026dbc 100644 --- a/include/envoy/event/dispatcher.h +++ b/include/envoy/event/dispatcher.h @@ -263,7 +263,7 @@ class Dispatcher : public DispatcherBase { /** * Similar to `post()` but return false if the dispatcher rejects. */ - virtual bool tryPost(PostCb callback) PURE; + virtual bool tryPost(PostCb&& callback) PURE; /** * Runs the event loop. This will not return until exit() is called either from within a callback diff --git a/source/common/event/dispatcher_impl.cc b/source/common/event/dispatcher_impl.cc index fa4852ae76f4..b4147fe724a2 100644 --- a/source/common/event/dispatcher_impl.cc +++ b/source/common/event/dispatcher_impl.cc @@ -249,15 +249,9 @@ void DispatcherImpl::exit() { { Thread::LockGuard lock(post_lock_); exited_ = true; - // No more post! + // No more tryPost! } base_scheduler_.loopExit(); - { - FANCY_LOG(debug, "lambdai: running postcallbacks after exit"); - // TODO(lambdai): runtime key. - runPostCallbacks(); - FANCY_LOG(debug, "lambdai: complete postcallbacks after exit"); - } } SignalEventPtr DispatcherImpl::listenForSignal(signal_t signal_num, SignalCb cb) { @@ -278,20 +272,21 @@ void DispatcherImpl::post(std::function callback) { } } -bool DispatcherImpl::tryPost(std::function callback) { +bool DispatcherImpl::tryPost(std::function&& callback) { bool do_post; - // Post master to master. Skip. + // Skip master to master post. if (isThreadSafe()) { return false; } { Thread::LockGuard lock(post_lock_); - // TODO(lambdai): For compatibility we should allow blind post. if (exited_) { return false; } do_post = post_callbacks_.empty(); post_callbacks_.push_back(callback); + // poor man's move only function. + callback = nullptr; } if (do_post) { @@ -304,7 +299,7 @@ void DispatcherImpl::run(RunType type) { run_tid_ = api_.threadFactory().currentThreadId(); { // Allows tryPost. - FANCY_LOG(debug, "lambdai: run {}", type); + ENVOY_LOG(debug, "{}({})", __FUNCTION__, type); Thread::LockGuard lock(post_lock_); if (type == RunType::Block) { exited_ = false; @@ -317,7 +312,7 @@ void DispatcherImpl::run(RunType type) { runPostCallbacks(); base_scheduler_.run(type); { - FANCY_LOG(debug, "lambdai: run return {}", type); + ENVOY_LOG(debug, "{}({})", __FUNCTION__, type); // Reject all the follow up tryPost. Thread::LockGuard lock(post_lock_); exited_ = true; diff --git a/source/common/event/dispatcher_impl.h b/source/common/event/dispatcher_impl.h index 8a1e902b441c..f97594924e8e 100644 --- a/source/common/event/dispatcher_impl.h +++ b/source/common/event/dispatcher_impl.h @@ -86,7 +86,7 @@ class DispatcherImpl : Logger::Loggable, void exit() override; SignalEventPtr listenForSignal(signal_t signal_num, SignalCb cb) override; void post(std::function callback) override; - bool tryPost(std::function callback) override; + bool tryPost(std::function&& callback) override; void run(RunType type) override; Buffer::WatermarkFactory& getWatermarkFactory() override { return *buffer_factory_; } void pushTrackedObject(const ScopeTrackedObject* object) override; diff --git a/source/common/upstream/upstream_impl.cc b/source/common/upstream/upstream_impl.cc index bef17f4b321e..22331ada74f9 100644 --- a/source/common/upstream/upstream_impl.cc +++ b/source/common/upstream/upstream_impl.cc @@ -925,26 +925,20 @@ ClusterImplBase::ClusterImplBase( std::move(socket_matcher), std::move(stats_scope), added_via_api, factory_context), [&dispatcher](const ClusterInfoImpl* self) { - FANCY_LOG(debug, "lambdai: schedule destroy cluster info {} on this thread", self->name()); - if (!dispatcher.tryPost([self]() { - // TODO(lambdai): Yet there is risk that master dispatcher receives the function but - // doesn't execute during the shutdown. We can either 1) Introduce folly::function - // which supports with unique_ptr capture and destroy cluster info by RAII, or 2) Call - // run post callback in master thread after no worker post back. - FANCY_LOG(debug, - "lambdai: execute destroy cluster info {} on this thread. Master thread is " - "expected.", - self->name()); - delete self; + const auto name = self->name(); + ENVOY_LOG(debug, "Schedule destroy cluster info {}", self->name()); + if (!dispatcher.tryPost([raii_cluster = std::shared_ptr(self)]() { + ENVOY_LOG(debug, "Destroying cluster info {}. This thread should be master thread.", + raii_cluster->name()); })) { - FANCY_LOG(debug, - "lambdai: cannot post. Has the master thread exited? Executing destroy cluster " - "info {} on this thread.", - self->name()); - delete self; + ENVOY_LOG(debug, + "Cannot post cluster destroy task on master thread. Has the master thread " + "exited? Destroying cluster " + "info {} anyway.", + name); } }); - + if ((info_->features() & ClusterInfoImpl::Features::USE_ALPN) && !raw_factory_pointer->supportsAlpn()) { throw EnvoyException( diff --git a/test/common/upstream/logical_dns_cluster_test.cc b/test/common/upstream/logical_dns_cluster_test.cc index caef2da1956b..57e63e9de27e 100644 --- a/test/common/upstream/logical_dns_cluster_test.cc +++ b/test/common/upstream/logical_dns_cluster_test.cc @@ -204,11 +204,11 @@ class LogicalDnsClusterTest : public Event::TestUsingSimulatedTime, public testi Network::DnsResolver::ResolveCb dns_callback_; NiceMock tls_; Event::MockTimer* resolve_timer_; - std::shared_ptr cluster_; ReadyWatcher membership_updated_; ReadyWatcher initialized_; NiceMock runtime_; NiceMock dispatcher_; + std::shared_ptr cluster_; NiceMock local_info_; NiceMock admin_; Singleton::ManagerImpl singleton_manager_{Thread::threadFactoryForTest()}; diff --git a/test/common/upstream/original_dst_cluster_test.cc b/test/common/upstream/original_dst_cluster_test.cc index 26f15b226453..9ecd87ede145 100644 --- a/test/common/upstream/original_dst_cluster_test.cc +++ b/test/common/upstream/original_dst_cluster_test.cc @@ -94,11 +94,11 @@ class OriginalDstClusterTest : public Event::TestUsingSimulatedTime, public test Stats::TestUtil::TestStore stats_store_; Ssl::MockContextManager ssl_context_manager_; + NiceMock dispatcher_; OriginalDstClusterSharedPtr cluster_; ReadyWatcher membership_updated_; ReadyWatcher initialized_; NiceMock runtime_; - NiceMock dispatcher_; Event::MockTimer* cleanup_timer_; NiceMock random_; NiceMock local_info_; diff --git a/test/mocks/event/mocks.h b/test/mocks/event/mocks.h index 9fc284c539d6..a8d63f204ee7 100644 --- a/test/mocks/event/mocks.h +++ b/test/mocks/event/mocks.h @@ -146,7 +146,7 @@ class MockDispatcher : public Dispatcher { MOCK_METHOD(void, exit, ()); MOCK_METHOD(SignalEvent*, listenForSignal_, (signal_t signal_num, SignalCb cb)); MOCK_METHOD(void, post, (std::function callback)); - MOCK_METHOD(bool, tryPost, (std::function callback)); + MOCK_METHOD(bool, tryPost, (std::function&& callback)); MOCK_METHOD(void, run, (RunType type)); MOCK_METHOD(void, pushTrackedObject, (const ScopeTrackedObject* object)); MOCK_METHOD(void, popTrackedObject, (const ScopeTrackedObject* expected_object)); diff --git a/test/mocks/event/wrapped_dispatcher.h b/test/mocks/event/wrapped_dispatcher.h index c23509e61b8e..d03ebf9b0d3e 100644 --- a/test/mocks/event/wrapped_dispatcher.h +++ b/test/mocks/event/wrapped_dispatcher.h @@ -101,7 +101,7 @@ class WrappedDispatcher : public Dispatcher { void post(std::function callback) override { impl_.post(std::move(callback)); } - bool tryPost(std::function callback) override { + bool tryPost(std::function&& callback) override { return impl_.tryPost(std::move(callback)); } From 8e380ce5e1bc538b7185e536e6c994a26ed0ca80 Mon Sep 17 00:00:00 2001 From: Yuchen Dai Date: Fri, 5 Feb 2021 04:02:58 -0800 Subject: [PATCH 08/40] remove extra runPostCallbacks() in run Signed-off-by: Yuchen Dai --- source/common/event/dispatcher_impl.cc | 2 -- 1 file changed, 2 deletions(-) diff --git a/source/common/event/dispatcher_impl.cc b/source/common/event/dispatcher_impl.cc index b4147fe724a2..2d50ca75bb03 100644 --- a/source/common/event/dispatcher_impl.cc +++ b/source/common/event/dispatcher_impl.cc @@ -317,8 +317,6 @@ void DispatcherImpl::run(RunType type) { Thread::LockGuard lock(post_lock_); exited_ = true; } - // TODO(lambdai): reconsider this. - runPostCallbacks(); } MonotonicTime DispatcherImpl::approximateMonotonicTime() const { From bcad62ed76a4d01e086c5c5aa4e464761ea95187 Mon Sep 17 00:00:00 2001 From: Yuchen Dai Date: Fri, 5 Feb 2021 04:12:35 -0800 Subject: [PATCH 09/40] remove exit flag in dispatcher Signed-off-by: Yuchen Dai --- source/common/event/dispatcher_impl.cc | 26 ++------------------------ source/common/event/dispatcher_impl.h | 2 +- 2 files changed, 3 insertions(+), 25 deletions(-) diff --git a/source/common/event/dispatcher_impl.cc b/source/common/event/dispatcher_impl.cc index 2d50ca75bb03..2f5201694cf7 100644 --- a/source/common/event/dispatcher_impl.cc +++ b/source/common/event/dispatcher_impl.cc @@ -246,11 +246,6 @@ void DispatcherImpl::deferredDelete(DeferredDeletablePtr&& to_delete) { } void DispatcherImpl::exit() { - { - Thread::LockGuard lock(post_lock_); - exited_ = true; - // No more tryPost! - } base_scheduler_.loopExit(); } @@ -280,12 +275,9 @@ bool DispatcherImpl::tryPost(std::function&& callback) { } { Thread::LockGuard lock(post_lock_); - if (exited_) { - return false; - } do_post = post_callbacks_.empty(); - post_callbacks_.push_back(callback); - // poor man's move only function. + post_callbacks_.push_back(std::move(callback)); + // Poor man's move only function. callback = nullptr; } @@ -297,26 +289,12 @@ bool DispatcherImpl::tryPost(std::function&& callback) { void DispatcherImpl::run(RunType type) { run_tid_ = api_.threadFactory().currentThreadId(); - { - // Allows tryPost. - ENVOY_LOG(debug, "{}({})", __FUNCTION__, type); - Thread::LockGuard lock(post_lock_); - if (type == RunType::Block) { - exited_ = false; - } - } // Flush all post callbacks before we run the event loop. We do this because there are post // callbacks that have to get run before the initial event loop starts running. libevent does // not guarantee that events are run in any particular order. So even if we post() and call // event_base_once() before some other event, the other event might get called first. runPostCallbacks(); base_scheduler_.run(type); - { - ENVOY_LOG(debug, "{}({})", __FUNCTION__, type); - // Reject all the follow up tryPost. - Thread::LockGuard lock(post_lock_); - exited_ = true; - } } MonotonicTime DispatcherImpl::approximateMonotonicTime() const { diff --git a/source/common/event/dispatcher_impl.h b/source/common/event/dispatcher_impl.h index f97594924e8e..189d7f66340d 100644 --- a/source/common/event/dispatcher_impl.h +++ b/source/common/event/dispatcher_impl.h @@ -137,7 +137,7 @@ class DispatcherImpl : Logger::Loggable, bool isThreadSafe() const override { return run_tid_.isEmpty() || run_tid_ == api_.threadFactory().currentThreadId(); } - std::atomic exited_{}; + const std::string name_; Api::Api& api_; std::string stats_prefix_; From 50ec1418c500bfa38d599c6ec6f687bb429592c6 Mon Sep 17 00:00:00 2001 From: Yuchen Dai Date: Fri, 5 Feb 2021 04:28:43 -0800 Subject: [PATCH 10/40] rename to movePost returning void, tests failing Signed-off-by: Yuchen Dai --- include/envoy/event/dispatcher.h | 4 ++-- source/common/event/dispatcher_impl.cc | 7 +------ source/common/event/dispatcher_impl.h | 2 +- source/common/upstream/upstream_impl.cc | 10 ++-------- test/common/upstream/cluster_manager_impl_test.cc | 2 +- test/mocks/event/mocks.h | 2 +- test/mocks/event/wrapped_dispatcher.h | 4 ++-- 7 files changed, 10 insertions(+), 21 deletions(-) diff --git a/include/envoy/event/dispatcher.h b/include/envoy/event/dispatcher.h index ea6a17026dbc..8190faf95d0f 100644 --- a/include/envoy/event/dispatcher.h +++ b/include/envoy/event/dispatcher.h @@ -261,9 +261,9 @@ class Dispatcher : public DispatcherBase { virtual void post(PostCb callback) PURE; /** - * Similar to `post()` but return false if the dispatcher rejects. + * Similar to `post()` but will destroy passed callback. This simulates posting move only function. */ - virtual bool tryPost(PostCb&& callback) PURE; + virtual void movePost(PostCb&& callback) PURE; /** * Runs the event loop. This will not return until exit() is called either from within a callback diff --git a/source/common/event/dispatcher_impl.cc b/source/common/event/dispatcher_impl.cc index 2f5201694cf7..c0fcb2149ace 100644 --- a/source/common/event/dispatcher_impl.cc +++ b/source/common/event/dispatcher_impl.cc @@ -267,12 +267,8 @@ void DispatcherImpl::post(std::function callback) { } } -bool DispatcherImpl::tryPost(std::function&& callback) { +void DispatcherImpl::movePost(std::function&& callback) { bool do_post; - // Skip master to master post. - if (isThreadSafe()) { - return false; - } { Thread::LockGuard lock(post_lock_); do_post = post_callbacks_.empty(); @@ -284,7 +280,6 @@ bool DispatcherImpl::tryPost(std::function&& callback) { if (do_post) { post_cb_->scheduleCallbackCurrentIteration(); } - return true; } void DispatcherImpl::run(RunType type) { diff --git a/source/common/event/dispatcher_impl.h b/source/common/event/dispatcher_impl.h index 189d7f66340d..201a294642d4 100644 --- a/source/common/event/dispatcher_impl.h +++ b/source/common/event/dispatcher_impl.h @@ -86,7 +86,7 @@ class DispatcherImpl : Logger::Loggable, void exit() override; SignalEventPtr listenForSignal(signal_t signal_num, SignalCb cb) override; void post(std::function callback) override; - bool tryPost(std::function&& callback) override; + void movePost(std::function&& callback) override; void run(RunType type) override; Buffer::WatermarkFactory& getWatermarkFactory() override { return *buffer_factory_; } void pushTrackedObject(const ScopeTrackedObject* object) override; diff --git a/source/common/upstream/upstream_impl.cc b/source/common/upstream/upstream_impl.cc index 22331ada74f9..6e2b7618e23e 100644 --- a/source/common/upstream/upstream_impl.cc +++ b/source/common/upstream/upstream_impl.cc @@ -927,16 +927,10 @@ ClusterImplBase::ClusterImplBase( [&dispatcher](const ClusterInfoImpl* self) { const auto name = self->name(); ENVOY_LOG(debug, "Schedule destroy cluster info {}", self->name()); - if (!dispatcher.tryPost([raii_cluster = std::shared_ptr(self)]() { + dispatcher.movePost([raii_cluster = std::shared_ptr(self)]() { ENVOY_LOG(debug, "Destroying cluster info {}. This thread should be master thread.", raii_cluster->name()); - })) { - ENVOY_LOG(debug, - "Cannot post cluster destroy task on master thread. Has the master thread " - "exited? Destroying cluster " - "info {} anyway.", - name); - } + }); }); if ((info_->features() & ClusterInfoImpl::Features::USE_ALPN) && diff --git a/test/common/upstream/cluster_manager_impl_test.cc b/test/common/upstream/cluster_manager_impl_test.cc index 4f165e93bfb1..e0d09d0acc18 100644 --- a/test/common/upstream/cluster_manager_impl_test.cc +++ b/test/common/upstream/cluster_manager_impl_test.cc @@ -874,7 +874,7 @@ TEST_F(ClusterManagerImplTest, HttpHealthChecker) { createClientConnection_( PointeesEq(Network::Utility::resolveUrl("tcp://127.0.0.1:11001")), _, _, _)) .WillOnce(Return(connection)); - EXPECT_CALL(factory_.dispatcher_, tryPost(_)).Times(1); + EXPECT_CALL(factory_.dispatcher_, movePost(_)).Times(1); create(parseBootstrapFromV3Yaml(yaml)); factory_.tls_.shutdownThread(); factory_.dispatcher_.to_delete_.clear(); diff --git a/test/mocks/event/mocks.h b/test/mocks/event/mocks.h index a8d63f204ee7..6326e85da262 100644 --- a/test/mocks/event/mocks.h +++ b/test/mocks/event/mocks.h @@ -146,7 +146,7 @@ class MockDispatcher : public Dispatcher { MOCK_METHOD(void, exit, ()); MOCK_METHOD(SignalEvent*, listenForSignal_, (signal_t signal_num, SignalCb cb)); MOCK_METHOD(void, post, (std::function callback)); - MOCK_METHOD(bool, tryPost, (std::function&& callback)); + MOCK_METHOD(void, movePost, (std::function&& callback)); MOCK_METHOD(void, run, (RunType type)); MOCK_METHOD(void, pushTrackedObject, (const ScopeTrackedObject* object)); MOCK_METHOD(void, popTrackedObject, (const ScopeTrackedObject* expected_object)); diff --git a/test/mocks/event/wrapped_dispatcher.h b/test/mocks/event/wrapped_dispatcher.h index d03ebf9b0d3e..020c77f7d7b8 100644 --- a/test/mocks/event/wrapped_dispatcher.h +++ b/test/mocks/event/wrapped_dispatcher.h @@ -101,8 +101,8 @@ class WrappedDispatcher : public Dispatcher { void post(std::function callback) override { impl_.post(std::move(callback)); } - bool tryPost(std::function&& callback) override { - return impl_.tryPost(std::move(callback)); + void movePost(std::function&& callback) override { + impl_.movePost(std::move(callback)); } void run(RunType type) override { impl_.run(type); } From 4746d5305bc0ff455c01ac7349421f488878cd2a Mon Sep 17 00:00:00 2001 From: Yuchen Dai Date: Fri, 5 Feb 2021 04:52:04 -0800 Subject: [PATCH 11/40] relax ssl manager required empty context Signed-off-by: Yuchen Dai --- .../extensions/transport_sockets/tls/context_manager_impl.cc | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/source/extensions/transport_sockets/tls/context_manager_impl.cc b/source/extensions/transport_sockets/tls/context_manager_impl.cc index fd0eefe761ea..758f03f5c238 100644 --- a/source/extensions/transport_sockets/tls/context_manager_impl.cc +++ b/source/extensions/transport_sockets/tls/context_manager_impl.cc @@ -17,7 +17,8 @@ namespace Tls { ContextManagerImpl::~ContextManagerImpl() { removeEmptyContexts(); - KNOWN_ISSUE_ASSERT(contexts_.empty(), "https://github.com/envoyproxy/envoy/issues/10030"); + // Contexts could be referenced by cluster to be destroyed. The cluster destroy is driven by the + // master thread dispatcher, but the dispatcher ceases running for good. } void ContextManagerImpl::removeEmptyContexts() { From 0ca27ecf42fad4bc4b78051f38fda8935795599f Mon Sep 17 00:00:00 2001 From: Yuchen Dai Date: Fri, 5 Feb 2021 04:53:27 -0800 Subject: [PATCH 12/40] format Signed-off-by: Yuchen Dai --- include/envoy/event/dispatcher.h | 3 ++- source/common/event/dispatcher_impl.cc | 4 +--- source/common/event/dispatcher_impl.h | 2 +- source/common/upstream/upstream_impl.cc | 6 +++--- test/common/upstream/cluster_manager_impl_test.cc | 2 +- test/mocks/event/mocks.h | 2 +- test/mocks/event/wrapped_dispatcher.h | 4 +--- 7 files changed, 10 insertions(+), 13 deletions(-) diff --git a/include/envoy/event/dispatcher.h b/include/envoy/event/dispatcher.h index 8190faf95d0f..14d7199db0f5 100644 --- a/include/envoy/event/dispatcher.h +++ b/include/envoy/event/dispatcher.h @@ -261,7 +261,8 @@ class Dispatcher : public DispatcherBase { virtual void post(PostCb callback) PURE; /** - * Similar to `post()` but will destroy passed callback. This simulates posting move only function. + * Similar to `post()` but will destroy passed callback. This simulates posting move only + * function. */ virtual void movePost(PostCb&& callback) PURE; diff --git a/source/common/event/dispatcher_impl.cc b/source/common/event/dispatcher_impl.cc index c0fcb2149ace..857235496642 100644 --- a/source/common/event/dispatcher_impl.cc +++ b/source/common/event/dispatcher_impl.cc @@ -245,9 +245,7 @@ void DispatcherImpl::deferredDelete(DeferredDeletablePtr&& to_delete) { } } -void DispatcherImpl::exit() { - base_scheduler_.loopExit(); -} +void DispatcherImpl::exit() { base_scheduler_.loopExit(); } SignalEventPtr DispatcherImpl::listenForSignal(signal_t signal_num, SignalCb cb) { ASSERT(isThreadSafe()); diff --git a/source/common/event/dispatcher_impl.h b/source/common/event/dispatcher_impl.h index 201a294642d4..4db6950cce6c 100644 --- a/source/common/event/dispatcher_impl.h +++ b/source/common/event/dispatcher_impl.h @@ -137,7 +137,7 @@ class DispatcherImpl : Logger::Loggable, bool isThreadSafe() const override { return run_tid_.isEmpty() || run_tid_ == api_.threadFactory().currentThreadId(); } - + const std::string name_; Api::Api& api_; std::string stats_prefix_; diff --git a/source/common/upstream/upstream_impl.cc b/source/common/upstream/upstream_impl.cc index 6e2b7618e23e..30cebd4be49f 100644 --- a/source/common/upstream/upstream_impl.cc +++ b/source/common/upstream/upstream_impl.cc @@ -928,9 +928,9 @@ ClusterImplBase::ClusterImplBase( const auto name = self->name(); ENVOY_LOG(debug, "Schedule destroy cluster info {}", self->name()); dispatcher.movePost([raii_cluster = std::shared_ptr(self)]() { - ENVOY_LOG(debug, "Destroying cluster info {}. This thread should be master thread.", - raii_cluster->name()); - }); + ENVOY_LOG(debug, "Destroying cluster info {}. This thread should be master thread.", + raii_cluster->name()); + }); }); if ((info_->features() & ClusterInfoImpl::Features::USE_ALPN) && diff --git a/test/common/upstream/cluster_manager_impl_test.cc b/test/common/upstream/cluster_manager_impl_test.cc index e0d09d0acc18..653370c7db5c 100644 --- a/test/common/upstream/cluster_manager_impl_test.cc +++ b/test/common/upstream/cluster_manager_impl_test.cc @@ -874,7 +874,7 @@ TEST_F(ClusterManagerImplTest, HttpHealthChecker) { createClientConnection_( PointeesEq(Network::Utility::resolveUrl("tcp://127.0.0.1:11001")), _, _, _)) .WillOnce(Return(connection)); - EXPECT_CALL(factory_.dispatcher_, movePost(_)).Times(1); + EXPECT_CALL(factory_.dispatcher_, movePost(_)); create(parseBootstrapFromV3Yaml(yaml)); factory_.tls_.shutdownThread(); factory_.dispatcher_.to_delete_.clear(); diff --git a/test/mocks/event/mocks.h b/test/mocks/event/mocks.h index 6326e85da262..0e8420dc5270 100644 --- a/test/mocks/event/mocks.h +++ b/test/mocks/event/mocks.h @@ -146,7 +146,7 @@ class MockDispatcher : public Dispatcher { MOCK_METHOD(void, exit, ()); MOCK_METHOD(SignalEvent*, listenForSignal_, (signal_t signal_num, SignalCb cb)); MOCK_METHOD(void, post, (std::function callback)); - MOCK_METHOD(void, movePost, (std::function&& callback)); + MOCK_METHOD(void, movePost, (std::function && callback)); MOCK_METHOD(void, run, (RunType type)); MOCK_METHOD(void, pushTrackedObject, (const ScopeTrackedObject* object)); MOCK_METHOD(void, popTrackedObject, (const ScopeTrackedObject* expected_object)); diff --git a/test/mocks/event/wrapped_dispatcher.h b/test/mocks/event/wrapped_dispatcher.h index 020c77f7d7b8..a8913ccdd146 100644 --- a/test/mocks/event/wrapped_dispatcher.h +++ b/test/mocks/event/wrapped_dispatcher.h @@ -101,9 +101,7 @@ class WrappedDispatcher : public Dispatcher { void post(std::function callback) override { impl_.post(std::move(callback)); } - void movePost(std::function&& callback) override { - impl_.movePost(std::move(callback)); - } + void movePost(std::function&& callback) override { impl_.movePost(std::move(callback)); } void run(RunType type) override { impl_.run(type); } From ec6eb4a84618e1581612d65c054b130aefac6a78 Mon Sep 17 00:00:00 2001 From: Yuchen Dai Date: Fri, 5 Feb 2021 04:56:26 -0800 Subject: [PATCH 13/40] revert accidentally touched files Signed-off-by: Yuchen Dai --- source/common/upstream/BUILD | 1 - source/common/upstream/cluster_manager_impl.cc | 1 - 2 files changed, 2 deletions(-) diff --git a/source/common/upstream/BUILD b/source/common/upstream/BUILD index a72b5f694ee8..edfe095f1f8a 100644 --- a/source/common/upstream/BUILD +++ b/source/common/upstream/BUILD @@ -436,7 +436,6 @@ envoy_cc_library( ":strict_dns_cluster_lib", ":upstream_includes", ":transport_socket_match_lib", - "//source/common/event:deferred_task", "@envoy_api//envoy/config/cluster/v3:pkg_cc_proto", "@envoy_api//envoy/config/core/v3:pkg_cc_proto", "@envoy_api//envoy/config/endpoint/v3:pkg_cc_proto", diff --git a/source/common/upstream/cluster_manager_impl.cc b/source/common/upstream/cluster_manager_impl.cc index b6db9b1dbeae..35ac22b48195 100644 --- a/source/common/upstream/cluster_manager_impl.cc +++ b/source/common/upstream/cluster_manager_impl.cc @@ -20,7 +20,6 @@ #include "common/common/assert.h" #include "common/common/enum_to_int.h" #include "common/common/fmt.h" -#include "common/common/macros.h" #include "common/common/utility.h" #include "common/config/new_grpc_mux_impl.h" #include "common/config/utility.h" From 2f26bf4c62546f9a86aa64cfd008ecbc321b19f6 Mon Sep 17 00:00:00 2001 From: Yuchen Dai Date: Fri, 5 Feb 2021 17:06:35 -0800 Subject: [PATCH 14/40] add preShutdown Signed-off-by: Yuchen Dai --- include/envoy/event/dispatcher.h | 2 ++ source/common/event/dispatcher_impl.cc | 8 ++++++++ source/common/event/dispatcher_impl.h | 1 + .../transport_sockets/tls/context_manager_impl.cc | 1 + source/server/config_validation/server.cc | 1 + source/server/server.cc | 1 + test/mocks/event/mocks.h | 1 + test/mocks/event/wrapped_dispatcher.h | 2 ++ 8 files changed, 17 insertions(+) diff --git a/include/envoy/event/dispatcher.h b/include/envoy/event/dispatcher.h index 14d7199db0f5..7406943dce2e 100644 --- a/include/envoy/event/dispatcher.h +++ b/include/envoy/event/dispatcher.h @@ -293,6 +293,8 @@ class Dispatcher : public DispatcherBase { * Updates approximate monotonic time to current value. */ virtual void updateApproximateMonotonicTime() PURE; + + virtual void preShutdown() PURE; }; using DispatcherPtr = std::unique_ptr; diff --git a/source/common/event/dispatcher_impl.cc b/source/common/event/dispatcher_impl.cc index 857235496642..c3b2ac1dce4b 100644 --- a/source/common/event/dispatcher_impl.cc +++ b/source/common/event/dispatcher_impl.cc @@ -294,6 +294,14 @@ MonotonicTime DispatcherImpl::approximateMonotonicTime() const { return approximate_monotonic_time_; } +void DispatcherImpl::preShutdown() { + ENVOY_LOG(debug, "Clearing post_callbacks in {}", __FUNCTION__); + { + Thread::LockGuard lock(post_lock_); + post_callbacks_.clear(); + } +} + void DispatcherImpl::updateApproximateMonotonicTime() { updateApproximateMonotonicTimeInternal(); } void DispatcherImpl::updateApproximateMonotonicTimeInternal() { diff --git a/source/common/event/dispatcher_impl.h b/source/common/event/dispatcher_impl.h index 4db6950cce6c..29e69060f387 100644 --- a/source/common/event/dispatcher_impl.h +++ b/source/common/event/dispatcher_impl.h @@ -93,6 +93,7 @@ class DispatcherImpl : Logger::Loggable, void popTrackedObject(const ScopeTrackedObject* expected_object) override; MonotonicTime approximateMonotonicTime() const override; void updateApproximateMonotonicTime() override; + void preShutdown() override; // FatalErrorInterface void onFatalError(std::ostream& os) const override; diff --git a/source/extensions/transport_sockets/tls/context_manager_impl.cc b/source/extensions/transport_sockets/tls/context_manager_impl.cc index 758f03f5c238..1a482b70355b 100644 --- a/source/extensions/transport_sockets/tls/context_manager_impl.cc +++ b/source/extensions/transport_sockets/tls/context_manager_impl.cc @@ -19,6 +19,7 @@ ContextManagerImpl::~ContextManagerImpl() { removeEmptyContexts(); // Contexts could be referenced by cluster to be destroyed. The cluster destroy is driven by the // master thread dispatcher, but the dispatcher ceases running for good. + KNOWN_ISSUE_ASSERT(contexts_.empty(), "https://github.com/envoyproxy/envoy/issues/10030"); } void ContextManagerImpl::removeEmptyContexts() { diff --git a/source/server/config_validation/server.cc b/source/server/config_validation/server.cc index 946c0253b4f5..0c6455cf5a45 100644 --- a/source/server/config_validation/server.cc +++ b/source/server/config_validation/server.cc @@ -115,6 +115,7 @@ void ValidationInstance::shutdown() { if (config_.clusterManager() != nullptr) { config_.clusterManager()->shutdown(); } + dispatcher_->preShutdown(); thread_local_.shutdownThread(); } diff --git a/source/server/server.cc b/source/server/server.cc index b7afdd253e25..77bb5b1a13e3 100644 --- a/source/server/server.cc +++ b/source/server/server.cc @@ -794,6 +794,7 @@ void InstanceImpl::terminate() { if (config_.clusterManager() != nullptr) { config_.clusterManager()->shutdown(); } + dispatcher_->preShutdown(); handler_.reset(); thread_local_.shutdownThread(); restarter_.shutdown(); diff --git a/test/mocks/event/mocks.h b/test/mocks/event/mocks.h index 0e8420dc5270..35c222eb623f 100644 --- a/test/mocks/event/mocks.h +++ b/test/mocks/event/mocks.h @@ -155,6 +155,7 @@ class MockDispatcher : public Dispatcher { MOCK_METHOD(Thread::ThreadId, getCurrentThreadId, ()); MOCK_METHOD(MonotonicTime, approximateMonotonicTime, (), (const)); MOCK_METHOD(void, updateApproximateMonotonicTime, ()); + MOCK_METHOD(void, preShutdown, ()); GlobalTimeSystem time_system_; std::list to_delete_; diff --git a/test/mocks/event/wrapped_dispatcher.h b/test/mocks/event/wrapped_dispatcher.h index a8913ccdd146..4b74de80343f 100644 --- a/test/mocks/event/wrapped_dispatcher.h +++ b/test/mocks/event/wrapped_dispatcher.h @@ -122,6 +122,8 @@ class WrappedDispatcher : public Dispatcher { bool isThreadSafe() const override { return impl_.isThreadSafe(); } + void preShutdown() override { impl.preShutdown(); } + protected: Dispatcher& impl_; }; From 63b063ad1d90a67c5e670c608c9a1a0c2c2e40a4 Mon Sep 17 00:00:00 2001 From: Yuchen Dai Date: Fri, 5 Feb 2021 20:39:00 -0800 Subject: [PATCH 15/40] fix typo Signed-off-by: Yuchen Dai --- test/mocks/event/wrapped_dispatcher.h | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/test/mocks/event/wrapped_dispatcher.h b/test/mocks/event/wrapped_dispatcher.h index 4b74de80343f..38cc1e1676fb 100644 --- a/test/mocks/event/wrapped_dispatcher.h +++ b/test/mocks/event/wrapped_dispatcher.h @@ -122,7 +122,7 @@ class WrappedDispatcher : public Dispatcher { bool isThreadSafe() const override { return impl_.isThreadSafe(); } - void preShutdown() override { impl.preShutdown(); } + void preShutdown() override { impl_.preShutdown(); } protected: Dispatcher& impl_; From 75c257a19033c1253856ad0faf23a84522c3051a Mon Sep 17 00:00:00 2001 From: Yuchen Dai Date: Fri, 5 Feb 2021 23:35:50 -0800 Subject: [PATCH 16/40] fixing server tests Signed-off-by: Yuchen Dai --- source/common/event/dispatcher_impl.cc | 5 ++++- source/common/event/dispatcher_impl.h | 6 ++++-- source/common/grpc/async_client_impl.cc | 1 + source/common/http/async_client_impl.cc | 4 ++++ source/common/network/connection_impl.cc | 4 ++++ source/server/config_validation/api.h | 2 +- source/server/config_validation/server.cc | 2 +- source/server/config_validation/server.h | 14 +++++++------- source/server/server.cc | 2 +- test/common/network/connection_impl_test.cc | 1 + test/mocks/event/mocks.cc | 1 + test/server/server_test.cc | 2 +- 12 files changed, 30 insertions(+), 14 deletions(-) diff --git a/source/common/event/dispatcher_impl.cc b/source/common/event/dispatcher_impl.cc index c3b2ac1dce4b..d9980274848f 100644 --- a/source/common/event/dispatcher_impl.cc +++ b/source/common/event/dispatcher_impl.cc @@ -74,7 +74,10 @@ DispatcherImpl::DispatcherImpl(const std::string& name, Api::Api& api, std::bind(&DispatcherImpl::updateApproximateMonotonicTime, this)); } -DispatcherImpl::~DispatcherImpl() { FatalErrorHandler::removeFatalErrorHandler(*this); } +DispatcherImpl::~DispatcherImpl() { + ENVOY_LOG(debug, "destroying dispatcher {}", name_); + FatalErrorHandler::removeFatalErrorHandler(*this); +} void DispatcherImpl::registerWatchdog(const Server::WatchDogSharedPtr& watchdog, std::chrono::milliseconds min_touch_interval) { diff --git a/source/common/event/dispatcher_impl.h b/source/common/event/dispatcher_impl.h index 29e69060f387..1c104f561811 100644 --- a/source/common/event/dispatcher_impl.h +++ b/source/common/event/dispatcher_impl.h @@ -149,11 +149,13 @@ class DispatcherImpl : Logger::Loggable, SchedulerPtr scheduler_; SchedulableCallbackPtr deferred_delete_cb_; SchedulableCallbackPtr post_cb_; + Thread::MutexBasicLockable post_lock_; + // `post_callback_` must be destroyed after `to_delete` because `to_delete` may post callbacks. + std::list> post_callbacks_ ABSL_GUARDED_BY(post_lock_); std::vector to_delete_1_; std::vector to_delete_2_; std::vector* current_to_delete_; - Thread::MutexBasicLockable post_lock_; - std::list> post_callbacks_ ABSL_GUARDED_BY(post_lock_); + absl::InlinedVector tracked_object_stack_; bool deferred_deleting_{}; diff --git a/source/common/grpc/async_client_impl.cc b/source/common/grpc/async_client_impl.cc index 9128b296ccb7..6d1152ef9f45 100644 --- a/source/common/grpc/async_client_impl.cc +++ b/source/common/grpc/async_client_impl.cc @@ -210,6 +210,7 @@ void AsyncStreamImpl::cleanup() { // This will destroy us, but only do so if we are actually in a list. This does not happen in // the immediate failure case. if (LinkedObject::inserted()) { + ASSERT(dispatcher_->isThreadSafe()); dispatcher_->deferredDelete( LinkedObject::removeFromList(parent_.active_streams_)); } diff --git a/source/common/http/async_client_impl.cc b/source/common/http/async_client_impl.cc index 361fdfb173ac..4eaf30f5c0ec 100644 --- a/source/common/http/async_client_impl.cc +++ b/source/common/http/async_client_impl.cc @@ -149,6 +149,7 @@ void AsyncStreamImpl::sendHeaders(RequestHeaderMap& headers, bool end_stream) { } void AsyncStreamImpl::sendData(Buffer::Instance& data, bool end_stream) { + ASSERT(dispatcher().isThreadSafe()); // Map send calls after local closure to no-ops. The send call could have been queued prior to // remote reset or closure, and/or closure could have occurred synchronously in response to a // previous send. In these cases the router will have already cleaned up stream state. This @@ -169,6 +170,8 @@ void AsyncStreamImpl::sendData(Buffer::Instance& data, bool end_stream) { } void AsyncStreamImpl::sendTrailers(RequestTrailerMap& trailers) { + ASSERT(dispatcher().isThreadSafe()); + // See explanation in sendData. if (local_closed_) { return; @@ -226,6 +229,7 @@ void AsyncStreamImpl::reset() { } void AsyncStreamImpl::cleanup() { + ASSERT(dispatcher().isThreadSafe()); local_closed_ = remote_closed_ = true; // This will destroy us, but only do so if we are actually in a list. This does not happen in // the immediate failure case. diff --git a/source/common/network/connection_impl.cc b/source/common/network/connection_impl.cc index 19389f2f1aed..0e55d73a487f 100644 --- a/source/common/network/connection_impl.cc +++ b/source/common/network/connection_impl.cc @@ -212,6 +212,7 @@ Connection::State ConnectionImpl::state() const { void ConnectionImpl::closeConnectionImmediately() { closeSocket(ConnectionEvent::LocalClose); } void ConnectionImpl::setTransportSocketIsReadable() { + ASSERT(dispatcher_.isThreadSafe()); // Remember that the transport requested read resumption, in case the resumption event is not // scheduled immediately or is "lost" because read was disabled. transport_wants_read_ = true; @@ -302,6 +303,7 @@ void ConnectionImpl::noDelay(bool enable) { } void ConnectionImpl::onRead(uint64_t read_buffer_size) { + ASSERT(dispatcher_.isThreadSafe()); if (inDelayedClose() || !filterChainWantsData()) { return; } @@ -421,6 +423,7 @@ void ConnectionImpl::raiseEvent(ConnectionEvent event) { bool ConnectionImpl::readEnabled() const { // Calls to readEnabled on a closed socket are considered to be an error. ASSERT(state() == State::Open); + ASSERT(dispatcher_.isThreadSafe()); return read_disable_count_ == 0; } @@ -438,6 +441,7 @@ void ConnectionImpl::write(Buffer::Instance& data, bool end_stream) { void ConnectionImpl::write(Buffer::Instance& data, bool end_stream, bool through_filter_chain) { ASSERT(!end_stream || enable_half_close_); + ASSERT(dispatcher_.isThreadSafe()); if (write_end_stream_) { // It is an API violation to write more data after writing end_stream, but a duplicate diff --git a/source/server/config_validation/api.h b/source/server/config_validation/api.h index 2d44022c191a..709d55b3bf6f 100644 --- a/source/server/config_validation/api.h +++ b/source/server/config_validation/api.h @@ -18,7 +18,7 @@ class ValidationImpl : public Impl { ValidationImpl(Thread::ThreadFactory& thread_factory, Stats::Store& stats_store, Event::TimeSystem& time_system, Filesystem::Instance& file_system, Random::RandomGenerator& random_generator); - + ~ValidationImpl() override { FANCY_LOG(debug, "In {}", __FUNCTION__); } Event::DispatcherPtr allocateDispatcher(const std::string& name) override; Event::DispatcherPtr allocateDispatcher(const std::string& name, const Event::ScaledRangeTimerManagerFactory&) override; diff --git a/source/server/config_validation/server.cc b/source/server/config_validation/server.cc index 0c6455cf5a45..c36072610b71 100644 --- a/source/server/config_validation/server.cc +++ b/source/server/config_validation/server.cc @@ -115,8 +115,8 @@ void ValidationInstance::shutdown() { if (config_.clusterManager() != nullptr) { config_.clusterManager()->shutdown(); } - dispatcher_->preShutdown(); thread_local_.shutdownThread(); + dispatcher_->preShutdown(); } } // namespace Server diff --git a/source/server/config_validation/server.h b/source/server/config_validation/server.h index e735b1ecd183..5e4cf6c9fbba 100644 --- a/source/server/config_validation/server.h +++ b/source/server/config_validation/server.h @@ -174,7 +174,12 @@ class ValidationInstance final : Logger::Loggable, void initialize(const Options& options, const Network::Address::InstanceConstSharedPtr& local_address, ComponentFactory& component_factory); - + const Options& options_; + ProtobufMessage::ProdValidationContextImpl validation_context_; + Stats::IsolatedStoreImpl& stats_store_; + Api::ApiPtr api_; + Event::DispatcherPtr dispatcher_; + ThreadLocal::InstanceImpl thread_local_; // init_manager_ must come before any member that participates in initialization, and destructed // only after referencing members are gone, since initialization continuation can potentially // occur at any point during member lifetime. @@ -186,12 +191,7 @@ class ValidationInstance final : Logger::Loggable, // - There may be active clusters referencing it in config_.cluster_manager_. // - There may be active connections referencing it. std::unique_ptr secret_manager_; - const Options& options_; - ProtobufMessage::ProdValidationContextImpl validation_context_; - Stats::IsolatedStoreImpl& stats_store_; - ThreadLocal::InstanceImpl thread_local_; - Api::ApiPtr api_; - Event::DispatcherPtr dispatcher_; + Server::ValidationAdmin admin_; Singleton::ManagerPtr singleton_manager_; std::unique_ptr runtime_singleton_; diff --git a/source/server/server.cc b/source/server/server.cc index 77bb5b1a13e3..9df8b58c7cd7 100644 --- a/source/server/server.cc +++ b/source/server/server.cc @@ -139,6 +139,7 @@ InstanceImpl::~InstanceImpl() { ENVOY_LOG(debug, "destroying listener manager"); listener_manager_.reset(); ENVOY_LOG(debug, "destroyed listener manager"); + dispatcher_->preShutdown(); } Upstream::ClusterManager& InstanceImpl::clusterManager() { @@ -794,7 +795,6 @@ void InstanceImpl::terminate() { if (config_.clusterManager() != nullptr) { config_.clusterManager()->shutdown(); } - dispatcher_->preShutdown(); handler_.reset(); thread_local_.shutdownThread(); restarter_.shutdown(); diff --git a/test/common/network/connection_impl_test.cc b/test/common/network/connection_impl_test.cc index 107bb77f60a3..76c6233ebb27 100644 --- a/test/common/network/connection_impl_test.cc +++ b/test/common/network/connection_impl_test.cc @@ -2004,6 +2004,7 @@ class FakeReadFilter : public Network::ReadFilter { class MockTransportConnectionImplTest : public testing::Test { public: MockTransportConnectionImplTest() : stream_info_(dispatcher_.timeSource(), nullptr) { + EXPECT_CALL(dispatcher_, isThreadSafe()).WillRepeatedly(Return(true)); EXPECT_CALL(dispatcher_.buffer_factory_, create_(_, _, _)) .WillRepeatedly(Invoke([](std::function below_low, std::function above_high, std::function above_overflow) -> Buffer::Instance* { diff --git a/test/mocks/event/mocks.cc b/test/mocks/event/mocks.cc index 75b60c1cbccb..c3f2c766e202 100644 --- a/test/mocks/event/mocks.cc +++ b/test/mocks/event/mocks.cc @@ -34,6 +34,7 @@ MockDispatcher::MockDispatcher(const std::string& name) : name_(name) { std::function above_overflow) -> Buffer::Instance* { return new Buffer::WatermarkBuffer(below_low, above_high, above_overflow); })); + ON_CALL(*this, isThreadSafe()).WillByDefault(Return(true)); } MockDispatcher::~MockDispatcher() = default; diff --git a/test/server/server_test.cc b/test/server/server_test.cc index 8695c0d0255d..90234fda8b97 100644 --- a/test/server/server_test.cc +++ b/test/server/server_test.cc @@ -268,8 +268,8 @@ class ServerInstanceImplTestBase { testing::NiceMock options_; DefaultListenerHooks hooks_; testing::NiceMock restart_; - ThreadLocal::InstanceImplPtr thread_local_; Stats::TestIsolatedStoreImpl stats_store_; + ThreadLocal::InstanceImplPtr thread_local_; Thread::MutexBasicLockable fakelock_; TestComponentFactory component_factory_; Event::GlobalTimeSystem time_system_; From dd4b1516e506f82b2c9c3638b12d6cf3c501156c Mon Sep 17 00:00:00 2001 From: Yuchen Dai Date: Sat, 6 Feb 2021 11:01:02 -0800 Subject: [PATCH 17/40] clang-tidy Signed-off-by: Yuchen Dai --- source/common/event/dispatcher_impl.cc | 2 +- source/common/http/async_client_impl.cc | 1 - source/common/upstream/upstream_impl.cc | 2 +- source/extensions/transport_sockets/tls/context_manager_impl.cc | 2 -- source/server/config_validation/api.h | 2 +- 5 files changed, 3 insertions(+), 6 deletions(-) diff --git a/source/common/event/dispatcher_impl.cc b/source/common/event/dispatcher_impl.cc index d9980274848f..f96396a30328 100644 --- a/source/common/event/dispatcher_impl.cc +++ b/source/common/event/dispatcher_impl.cc @@ -298,7 +298,7 @@ MonotonicTime DispatcherImpl::approximateMonotonicTime() const { } void DispatcherImpl::preShutdown() { - ENVOY_LOG(debug, "Clearing post_callbacks in {}", __FUNCTION__); + ENVOY_LOG(debug, "Clearing post callbacks in {}", __FUNCTION__); { Thread::LockGuard lock(post_lock_); post_callbacks_.clear(); diff --git a/source/common/http/async_client_impl.cc b/source/common/http/async_client_impl.cc index 4eaf30f5c0ec..ffe65b5ed62f 100644 --- a/source/common/http/async_client_impl.cc +++ b/source/common/http/async_client_impl.cc @@ -171,7 +171,6 @@ void AsyncStreamImpl::sendData(Buffer::Instance& data, bool end_stream) { void AsyncStreamImpl::sendTrailers(RequestTrailerMap& trailers) { ASSERT(dispatcher().isThreadSafe()); - // See explanation in sendData. if (local_closed_) { return; diff --git a/source/common/upstream/upstream_impl.cc b/source/common/upstream/upstream_impl.cc index 30cebd4be49f..318e1379049f 100644 --- a/source/common/upstream/upstream_impl.cc +++ b/source/common/upstream/upstream_impl.cc @@ -924,8 +924,8 @@ ClusterImplBase::ClusterImplBase( new ClusterInfoImpl(cluster, factory_context.clusterManager().bindConfig(), runtime, std::move(socket_matcher), std::move(stats_scope), added_via_api, factory_context), + // NOLINTNEXTLINE(clang-analyzer-cplusplus.NewDeleteLeaks) [&dispatcher](const ClusterInfoImpl* self) { - const auto name = self->name(); ENVOY_LOG(debug, "Schedule destroy cluster info {}", self->name()); dispatcher.movePost([raii_cluster = std::shared_ptr(self)]() { ENVOY_LOG(debug, "Destroying cluster info {}. This thread should be master thread.", diff --git a/source/extensions/transport_sockets/tls/context_manager_impl.cc b/source/extensions/transport_sockets/tls/context_manager_impl.cc index 1a482b70355b..fd0eefe761ea 100644 --- a/source/extensions/transport_sockets/tls/context_manager_impl.cc +++ b/source/extensions/transport_sockets/tls/context_manager_impl.cc @@ -17,8 +17,6 @@ namespace Tls { ContextManagerImpl::~ContextManagerImpl() { removeEmptyContexts(); - // Contexts could be referenced by cluster to be destroyed. The cluster destroy is driven by the - // master thread dispatcher, but the dispatcher ceases running for good. KNOWN_ISSUE_ASSERT(contexts_.empty(), "https://github.com/envoyproxy/envoy/issues/10030"); } diff --git a/source/server/config_validation/api.h b/source/server/config_validation/api.h index 709d55b3bf6f..2d44022c191a 100644 --- a/source/server/config_validation/api.h +++ b/source/server/config_validation/api.h @@ -18,7 +18,7 @@ class ValidationImpl : public Impl { ValidationImpl(Thread::ThreadFactory& thread_factory, Stats::Store& stats_store, Event::TimeSystem& time_system, Filesystem::Instance& file_system, Random::RandomGenerator& random_generator); - ~ValidationImpl() override { FANCY_LOG(debug, "In {}", __FUNCTION__); } + Event::DispatcherPtr allocateDispatcher(const std::string& name) override; Event::DispatcherPtr allocateDispatcher(const std::string& name, const Event::ScaledRangeTimerManagerFactory&) override; From 968a61188a0b106e0e934e8dbcf0bb7e2de4b793 Mon Sep 17 00:00:00 2001 From: Yuchen Dai Date: Sun, 7 Feb 2021 23:25:04 -0800 Subject: [PATCH 18/40] clang-tidy another try Signed-off-by: Yuchen Dai --- source/common/upstream/upstream_impl.cc | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/source/common/upstream/upstream_impl.cc b/source/common/upstream/upstream_impl.cc index 318e1379049f..112bf9decbdd 100644 --- a/source/common/upstream/upstream_impl.cc +++ b/source/common/upstream/upstream_impl.cc @@ -924,13 +924,13 @@ ClusterImplBase::ClusterImplBase( new ClusterInfoImpl(cluster, factory_context.clusterManager().bindConfig(), runtime, std::move(socket_matcher), std::move(stats_scope), added_via_api, factory_context), - // NOLINTNEXTLINE(clang-analyzer-cplusplus.NewDeleteLeaks) [&dispatcher](const ClusterInfoImpl* self) { ENVOY_LOG(debug, "Schedule destroy cluster info {}", self->name()); dispatcher.movePost([raii_cluster = std::shared_ptr(self)]() { ENVOY_LOG(debug, "Destroying cluster info {}. This thread should be master thread.", raii_cluster->name()); }); + // NOLINTNEXTLINE(clang-analyzer-cplusplus.NewDeleteLeaks) }); if ((info_->features() & ClusterInfoImpl::Features::USE_ALPN) && From d4dc8f8226e1f17e794fcdfd892f371728efddef Mon Sep 17 00:00:00 2001 From: Yuchen Dai Date: Mon, 8 Feb 2021 10:32:05 -0800 Subject: [PATCH 19/40] fix format Signed-off-by: Yuchen Dai --- source/common/upstream/upstream_impl.cc | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/source/common/upstream/upstream_impl.cc b/source/common/upstream/upstream_impl.cc index 112bf9decbdd..e0ce72fbb41b 100644 --- a/source/common/upstream/upstream_impl.cc +++ b/source/common/upstream/upstream_impl.cc @@ -930,7 +930,7 @@ ClusterImplBase::ClusterImplBase( ENVOY_LOG(debug, "Destroying cluster info {}. This thread should be master thread.", raii_cluster->name()); }); - // NOLINTNEXTLINE(clang-analyzer-cplusplus.NewDeleteLeaks) + // NOLINTNEXTLINE(clang-analyzer-cplusplus.NewDeleteLeaks) }); if ((info_->features() & ClusterInfoImpl::Features::USE_ALPN) && From ea86eb1f2146b27048fc73ea0eccba2db36e90a6 Mon Sep 17 00:00:00 2001 From: Yuchen Dai Date: Tue, 9 Feb 2021 04:01:44 -0800 Subject: [PATCH 20/40] rename shutdown, add integration test Signed-off-by: Yuchen Dai --- include/envoy/event/dispatcher.h | 5 +- source/common/event/dispatcher_impl.cc | 2 +- source/common/event/dispatcher_impl.h | 2 +- source/common/upstream/upstream_impl.cc | 16 ++- source/server/config_validation/server.cc | 2 +- source/server/server.cc | 2 +- .../sds_dynamic_integration_test.cc | 123 ++++++++++++++++++ test/mocks/event/mocks.h | 2 +- test/mocks/event/wrapped_dispatcher.h | 2 +- 9 files changed, 142 insertions(+), 14 deletions(-) diff --git a/include/envoy/event/dispatcher.h b/include/envoy/event/dispatcher.h index 7406943dce2e..f48521620f0c 100644 --- a/include/envoy/event/dispatcher.h +++ b/include/envoy/event/dispatcher.h @@ -294,7 +294,10 @@ class Dispatcher : public DispatcherBase { */ virtual void updateApproximateMonotonicTime() PURE; - virtual void preShutdown() PURE; + /** + * Shutdown the dispatcher by removing posted callbacks. + */ + virtual void shutdown() PURE; }; using DispatcherPtr = std::unique_ptr; diff --git a/source/common/event/dispatcher_impl.cc b/source/common/event/dispatcher_impl.cc index f96396a30328..35414ea6bc58 100644 --- a/source/common/event/dispatcher_impl.cc +++ b/source/common/event/dispatcher_impl.cc @@ -297,7 +297,7 @@ MonotonicTime DispatcherImpl::approximateMonotonicTime() const { return approximate_monotonic_time_; } -void DispatcherImpl::preShutdown() { +void DispatcherImpl::shutdown() { ENVOY_LOG(debug, "Clearing post callbacks in {}", __FUNCTION__); { Thread::LockGuard lock(post_lock_); diff --git a/source/common/event/dispatcher_impl.h b/source/common/event/dispatcher_impl.h index 1c104f561811..3a16741eea95 100644 --- a/source/common/event/dispatcher_impl.h +++ b/source/common/event/dispatcher_impl.h @@ -93,7 +93,7 @@ class DispatcherImpl : Logger::Loggable, void popTrackedObject(const ScopeTrackedObject* expected_object) override; MonotonicTime approximateMonotonicTime() const override; void updateApproximateMonotonicTime() override; - void preShutdown() override; + void shutdown() override; // FatalErrorInterface void onFatalError(std::ostream& os) const override; diff --git a/source/common/upstream/upstream_impl.cc b/source/common/upstream/upstream_impl.cc index e0ce72fbb41b..be1627177819 100644 --- a/source/common/upstream/upstream_impl.cc +++ b/source/common/upstream/upstream_impl.cc @@ -926,10 +926,12 @@ ClusterImplBase::ClusterImplBase( factory_context), [&dispatcher](const ClusterInfoImpl* self) { ENVOY_LOG(debug, "Schedule destroy cluster info {}", self->name()); - dispatcher.movePost([raii_cluster = std::shared_ptr(self)]() { - ENVOY_LOG(debug, "Destroying cluster info {}. This thread should be master thread.", - raii_cluster->name()); - }); + dispatcher.movePost( + [raii_cluster = std::shared_ptr(self)]() mutable { + ENVOY_LOG(debug, "Destroying cluster info {}. This thread should be master thread.", + raii_cluster->name()); + raii_cluster = nullptr; + }); // NOLINTNEXTLINE(clang-analyzer-cplusplus.NewDeleteLeaks) }); @@ -1119,7 +1121,7 @@ void ClusterImplBase::reloadHealthyHostsHelper(const HostSharedPtr&) { for (size_t priority = 0; priority < host_sets.size(); ++priority) { const auto& host_set = host_sets[priority]; // TODO(htuch): Can we skip these copies by exporting out const shared_ptr from HostSet? - HostVectorConstSharedPtr hosts_copy(new HostVector(host_set->hosts())); + HostVectorConstSharedPtr hosts_copy = std::make_shared(host_set->hosts()); HostsPerLocalityConstSharedPtr hosts_per_locality_copy = host_set->hostsPerLocality().clone(); prioritySet().updateHosts(priority, @@ -1310,10 +1312,10 @@ void PriorityStateManager::registerHostForPriority( auto metadata = lb_endpoint.has_metadata() ? parent_.constMetadataSharedPool()->getObject(lb_endpoint.metadata()) : nullptr; - const HostSharedPtr host(new HostImpl( + const auto host = std::make_shared( parent_.info(), hostname, address, metadata, lb_endpoint.load_balancing_weight().value(), locality_lb_endpoint.locality(), lb_endpoint.endpoint().health_check_config(), - locality_lb_endpoint.priority(), lb_endpoint.health_status(), time_source)); + locality_lb_endpoint.priority(), lb_endpoint.health_status(), time_source); registerHostForPriority(host, locality_lb_endpoint); } diff --git a/source/server/config_validation/server.cc b/source/server/config_validation/server.cc index c36072610b71..9a3fc6dc61b2 100644 --- a/source/server/config_validation/server.cc +++ b/source/server/config_validation/server.cc @@ -116,7 +116,7 @@ void ValidationInstance::shutdown() { config_.clusterManager()->shutdown(); } thread_local_.shutdownThread(); - dispatcher_->preShutdown(); + dispatcher_->shutdown(); } } // namespace Server diff --git a/source/server/server.cc b/source/server/server.cc index 9df8b58c7cd7..9f41a126d67f 100644 --- a/source/server/server.cc +++ b/source/server/server.cc @@ -139,7 +139,7 @@ InstanceImpl::~InstanceImpl() { ENVOY_LOG(debug, "destroying listener manager"); listener_manager_.reset(); ENVOY_LOG(debug, "destroyed listener manager"); - dispatcher_->preShutdown(); + dispatcher_->shutdown(); } Upstream::ClusterManager& InstanceImpl::clusterManager() { diff --git a/test/integration/sds_dynamic_integration_test.cc b/test/integration/sds_dynamic_integration_test.cc index 6023882ab9d6..20fa9fc03194 100644 --- a/test/integration/sds_dynamic_integration_test.cc +++ b/test/integration/sds_dynamic_integration_test.cc @@ -673,5 +673,128 @@ TEST_P(SdsDynamicUpstreamIntegrationTest, WrongSecretFirst) { EXPECT_EQ(1, test_server_->counter("sds.client_cert.update_rejected")->value()); } +// Test CDS with SDS. +class SdsCdsIntegrationTest : public SdsDynamicIntegrationBaseTest { +public: + void initialize() override { + config_helper_.addConfigModifier([this](envoy::config::bootstrap::v3::Bootstrap& bootstrap) { + // Create the dynamic cluster. This cluster will be using sds. + dynamic_cluster_ = bootstrap.mutable_static_resources()->clusters(0); + dynamic_cluster_.set_name("dynamic"); + dynamic_cluster_.mutable_connect_timeout()->MergeFrom( + ProtobufUtil::TimeUtil::MillisecondsToDuration(500000)); + auto* transport_socket = dynamic_cluster_.mutable_transport_socket(); + envoy::extensions::transport_sockets::tls::v3::UpstreamTlsContext tls_context; + auto* secret_config = + tls_context.mutable_common_tls_context()->add_tls_certificate_sds_secret_configs(); + setUpSdsConfig(secret_config, "client_cert"); + + transport_socket->set_name("envoy.transport_sockets.tls"); + transport_socket->mutable_typed_config()->PackFrom(tls_context); + + // Add cds cluster first. + auto* cds_cluster = bootstrap.mutable_static_resources()->add_clusters(); + cds_cluster->MergeFrom(bootstrap.static_resources().clusters()[0]); + cds_cluster->set_name("cds_cluster"); + ConfigHelper::setHttp2(*cds_cluster); + // Then add sds cluster first. + auto* sds_cluster = bootstrap.mutable_static_resources()->add_clusters(); + sds_cluster->MergeFrom(bootstrap.static_resources().clusters()[0]); + sds_cluster->set_name("sds_cluster"); + ConfigHelper::setHttp2(*sds_cluster); + + const std::string cds_yaml = R"EOF( + resource_api_version: V3 + api_config_source: + api_type: GRPC + transport_api_version: V3 + grpc_services: + envoy_grpc: + cluster_name: cds_cluster + set_node_on_first_message_only: true +)EOF"; + auto* cds = bootstrap.mutable_dynamic_resources()->mutable_cds_config(); + TestUtility::loadFromYaml(cds_yaml, *cds); + }); + + HttpIntegrationTest::initialize(); + // registerTestServerPorts({"http"}); + } + + void TearDown() override { + cleanUpXdsConnection(); + cleanupUpstreamAndDownstream(); + codec_client_.reset(); + test_server_.reset(); + fake_upstreams_.clear(); + } + + void createUpstreams() override { + // Static cluster. + addFakeUpstream(FakeHttpConnection::Type::HTTP1); + // Cds Cluster. + addFakeUpstream(FakeHttpConnection::Type::HTTP2); + // Sds Cluster. + addFakeUpstream(FakeHttpConnection::Type::HTTP2); + } + + void sendCdsResponse() { + EXPECT_TRUE(compareDiscoveryRequest(Config::TypeUrl::get().Cluster, "", {}, {}, {}, true)); + sendDiscoveryResponse( + Config::TypeUrl::get().Cluster, {dynamic_cluster_}, {dynamic_cluster_}, {}, "55"); + } + + void sendSdsResponse2(const envoy::extensions::transport_sockets::tls::v3::Secret& secret, + FakeStream& sds_stream) { + API_NO_BOOST(envoy::api::v2::DiscoveryResponse) discovery_response; + discovery_response.set_version_info("1"); + discovery_response.set_type_url(Config::TypeUrl::get().Secret); + discovery_response.add_resources()->PackFrom(secret); + sds_stream.sendGrpcMessage(discovery_response); + } + envoy::config::cluster::v3::Cluster dynamic_cluster_; + FakeHttpConnectionPtr sds_connection_; + FakeStreamPtr sds_stream_; +}; + +INSTANTIATE_TEST_SUITE_P(IpVersions, SdsCdsIntegrationTest, GRPC_CLIENT_INTEGRATION_PARAMS); + +TEST_P(SdsCdsIntegrationTest, BasicSuccess) { + on_server_init_function_ = [this]() { + { + // CDS. + AssertionResult result = + fake_upstreams_[1]->waitForHttpConnection(*dispatcher_, xds_connection_); + RELEASE_ASSERT(result, result.message()); + result = xds_connection_->waitForNewStream(*dispatcher_, xds_stream_); + RELEASE_ASSERT(result, result.message()); + xds_stream_->startGrpcStream(); + sendCdsResponse(); + } + { + // SDS. + AssertionResult result = + fake_upstreams_[2]->waitForHttpConnection(*dispatcher_, sds_connection_); + RELEASE_ASSERT(result, result.message()); + + result = sds_connection_->waitForNewStream(*dispatcher_, sds_stream_); + RELEASE_ASSERT(result, result.message()); + sds_stream_->startGrpcStream(); + sendSdsResponse2(getClientSecret(), *sds_stream_); + } + }; + initialize(); + + test_server_->waitForCounterGe( + "cluster.dynamic.client_ssl_socket_factory.ssl_context_update_by_sds", 1); + // The 4 clusters are CDS,SDS,static and dynamic cluster. + test_server_->waitForGaugeGe("cluster_manager.active_clusters", 4); + + sendDiscoveryResponse(Config::TypeUrl::get().Cluster, {}, {}, + {}, "42"); + // Successfully removed the dynamic cluster. + test_server_->waitForGaugeEq("cluster_manager.active_clusters", 3); +} + } // namespace Ssl } // namespace Envoy diff --git a/test/mocks/event/mocks.h b/test/mocks/event/mocks.h index 35c222eb623f..5c6ebafd040f 100644 --- a/test/mocks/event/mocks.h +++ b/test/mocks/event/mocks.h @@ -155,7 +155,7 @@ class MockDispatcher : public Dispatcher { MOCK_METHOD(Thread::ThreadId, getCurrentThreadId, ()); MOCK_METHOD(MonotonicTime, approximateMonotonicTime, (), (const)); MOCK_METHOD(void, updateApproximateMonotonicTime, ()); - MOCK_METHOD(void, preShutdown, ()); + MOCK_METHOD(void, shutdown, ()); GlobalTimeSystem time_system_; std::list to_delete_; diff --git a/test/mocks/event/wrapped_dispatcher.h b/test/mocks/event/wrapped_dispatcher.h index 38cc1e1676fb..5bb600af035a 100644 --- a/test/mocks/event/wrapped_dispatcher.h +++ b/test/mocks/event/wrapped_dispatcher.h @@ -122,7 +122,7 @@ class WrappedDispatcher : public Dispatcher { bool isThreadSafe() const override { return impl_.isThreadSafe(); } - void preShutdown() override { impl_.preShutdown(); } + void shutdown() override { impl_.shutdown(); } protected: Dispatcher& impl_; From 187179b0c801e25a2ed12da8bc767ff45bf05191 Mon Sep 17 00:00:00 2001 From: Yuchen Dai Date: Tue, 9 Feb 2021 05:06:43 -0800 Subject: [PATCH 21/40] clean up movePost Signed-off-by: Yuchen Dai --- include/envoy/event/dispatcher.h | 8 ++++---- source/common/event/dispatcher_impl.cc | 4 ++-- source/common/event/dispatcher_impl.h | 4 ++-- source/common/upstream/upstream_impl.cc | 11 +++++------ test/common/upstream/cluster_manager_impl_test.cc | 2 +- test/mocks/event/mocks.cc | 6 +++++- test/mocks/event/mocks.h | 4 ++-- test/mocks/event/wrapped_dispatcher.h | 4 ++-- 8 files changed, 23 insertions(+), 20 deletions(-) diff --git a/include/envoy/event/dispatcher.h b/include/envoy/event/dispatcher.h index f48521620f0c..fed798270de6 100644 --- a/include/envoy/event/dispatcher.h +++ b/include/envoy/event/dispatcher.h @@ -258,13 +258,13 @@ class Dispatcher : public DispatcherBase { * Posts a functor to the dispatcher. This is safe cross thread. The functor runs in the context * of the dispatcher event loop which may be on a different thread than the caller. */ - virtual void post(PostCb callback) PURE; + virtual void post(const PostCb& callback) PURE; /** - * Similar to `post()` but will destroy passed callback. This simulates posting move only - * function. + * Similar to `post(const PostCb&)` but will destroy passed callback. This simulates posting move + * only function. */ - virtual void movePost(PostCb&& callback) PURE; + virtual void post(PostCb&& callback) PURE; /** * Runs the event loop. This will not return until exit() is called either from within a callback diff --git a/source/common/event/dispatcher_impl.cc b/source/common/event/dispatcher_impl.cc index 35414ea6bc58..9d3398727164 100644 --- a/source/common/event/dispatcher_impl.cc +++ b/source/common/event/dispatcher_impl.cc @@ -255,7 +255,7 @@ SignalEventPtr DispatcherImpl::listenForSignal(signal_t signal_num, SignalCb cb) return SignalEventPtr{new SignalEventImpl(*this, signal_num, cb)}; } -void DispatcherImpl::post(std::function callback) { +void DispatcherImpl::post(const PostCb& callback) { bool do_post; { Thread::LockGuard lock(post_lock_); @@ -268,7 +268,7 @@ void DispatcherImpl::post(std::function callback) { } } -void DispatcherImpl::movePost(std::function&& callback) { +void DispatcherImpl::post(PostCb&& callback) { bool do_post; { Thread::LockGuard lock(post_lock_); diff --git a/source/common/event/dispatcher_impl.h b/source/common/event/dispatcher_impl.h index 3a16741eea95..76a902000dfb 100644 --- a/source/common/event/dispatcher_impl.h +++ b/source/common/event/dispatcher_impl.h @@ -85,8 +85,8 @@ class DispatcherImpl : Logger::Loggable, void deferredDelete(DeferredDeletablePtr&& to_delete) override; void exit() override; SignalEventPtr listenForSignal(signal_t signal_num, SignalCb cb) override; - void post(std::function callback) override; - void movePost(std::function&& callback) override; + void post(const PostCb& callback) override; + void post(PostCb&& callback) override; void run(RunType type) override; Buffer::WatermarkFactory& getWatermarkFactory() override { return *buffer_factory_; } void pushTrackedObject(const ScopeTrackedObject* object) override; diff --git a/source/common/upstream/upstream_impl.cc b/source/common/upstream/upstream_impl.cc index be1627177819..a5ab0c92f769 100644 --- a/source/common/upstream/upstream_impl.cc +++ b/source/common/upstream/upstream_impl.cc @@ -926,12 +926,11 @@ ClusterImplBase::ClusterImplBase( factory_context), [&dispatcher](const ClusterInfoImpl* self) { ENVOY_LOG(debug, "Schedule destroy cluster info {}", self->name()); - dispatcher.movePost( - [raii_cluster = std::shared_ptr(self)]() mutable { - ENVOY_LOG(debug, "Destroying cluster info {}. This thread should be master thread.", - raii_cluster->name()); - raii_cluster = nullptr; - }); + dispatcher.post([raii_cluster = std::shared_ptr(self)]() mutable { + ENVOY_LOG(debug, "Destroying cluster info {}. This thread should be master thread.", + raii_cluster->name()); + raii_cluster = nullptr; + }); // NOLINTNEXTLINE(clang-analyzer-cplusplus.NewDeleteLeaks) }); diff --git a/test/common/upstream/cluster_manager_impl_test.cc b/test/common/upstream/cluster_manager_impl_test.cc index 653370c7db5c..094bf420c664 100644 --- a/test/common/upstream/cluster_manager_impl_test.cc +++ b/test/common/upstream/cluster_manager_impl_test.cc @@ -874,7 +874,7 @@ TEST_F(ClusterManagerImplTest, HttpHealthChecker) { createClientConnection_( PointeesEq(Network::Utility::resolveUrl("tcp://127.0.0.1:11001")), _, _, _)) .WillOnce(Return(connection)); - EXPECT_CALL(factory_.dispatcher_, movePost(_)); + EXPECT_CALL(factory_.dispatcher_, post(_)); create(parseBootstrapFromV3Yaml(yaml)); factory_.tls_.shutdownThread(); factory_.dispatcher_.to_delete_.clear(); diff --git a/test/mocks/event/mocks.cc b/test/mocks/event/mocks.cc index c3f2c766e202..fae17cebddc4 100644 --- a/test/mocks/event/mocks.cc +++ b/test/mocks/event/mocks.cc @@ -27,7 +27,11 @@ MockDispatcher::MockDispatcher(const std::string& name) : name_(name) { ON_CALL(*this, createScaledTimer_(_, _)).WillByDefault(ReturnNew>()); ON_CALL(*this, createScaledTypedTimer_(_, _)) .WillByDefault(ReturnNew>()); - ON_CALL(*this, post(_)).WillByDefault(Invoke([](PostCb cb) -> void { cb(); })); + ON_CALL(*this, post(testing::Matcher())) + .WillByDefault(Invoke([](const PostCb& cb) -> void { cb(); })); + ON_CALL(*this, post(testing::Matcher())).WillByDefault(Invoke([](PostCb&& cb) -> void { + cb(); + })); ON_CALL(buffer_factory_, create_(_, _, _)) .WillByDefault(Invoke([](std::function below_low, std::function above_high, diff --git a/test/mocks/event/mocks.h b/test/mocks/event/mocks.h index 5c6ebafd040f..837fe54e064d 100644 --- a/test/mocks/event/mocks.h +++ b/test/mocks/event/mocks.h @@ -145,8 +145,8 @@ class MockDispatcher : public Dispatcher { MOCK_METHOD(void, deferredDelete_, (DeferredDeletable * to_delete)); MOCK_METHOD(void, exit, ()); MOCK_METHOD(SignalEvent*, listenForSignal_, (signal_t signal_num, SignalCb cb)); - MOCK_METHOD(void, post, (std::function callback)); - MOCK_METHOD(void, movePost, (std::function && callback)); + MOCK_METHOD(void, post, (const PostCb& callback)); + MOCK_METHOD(void, post, (PostCb && callback)); MOCK_METHOD(void, run, (RunType type)); MOCK_METHOD(void, pushTrackedObject, (const ScopeTrackedObject* object)); MOCK_METHOD(void, popTrackedObject, (const ScopeTrackedObject* expected_object)); diff --git a/test/mocks/event/wrapped_dispatcher.h b/test/mocks/event/wrapped_dispatcher.h index 5bb600af035a..a53868329ddd 100644 --- a/test/mocks/event/wrapped_dispatcher.h +++ b/test/mocks/event/wrapped_dispatcher.h @@ -99,9 +99,9 @@ class WrappedDispatcher : public Dispatcher { return impl_.listenForSignal(signal_num, std::move(cb)); } - void post(std::function callback) override { impl_.post(std::move(callback)); } + void post(const PostCb& callback) override { impl_.post(callback); } - void movePost(std::function&& callback) override { impl_.movePost(std::move(callback)); } + void post(PostCb&& callback) override { impl_.post(std::move(callback)); } void run(RunType type) override { impl_.run(type); } From 7782047509bd7abf94d4a7f0642eac0c4adec239 Mon Sep 17 00:00:00 2001 From: Yuchen Dai Date: Tue, 9 Feb 2021 12:33:11 -0800 Subject: [PATCH 22/40] fix ambigious Signed-off-by: Yuchen Dai --- test/common/router/rds_impl_test.cc | 3 +- test/common/router/scoped_rds_test.cc | 18 ++++---- test/common/stats/thread_local_store_test.cc | 2 +- .../thread_local/thread_local_impl_test.cc | 34 +++++++-------- .../upstream/cluster_manager_impl_test.cc | 2 +- .../upstream/health_checker_impl_test.cc | 6 ++- .../upstream/original_dst_cluster_test.cc | 41 +++++++++++-------- .../upstream/outlier_detection_impl_test.cc | 9 ++-- .../dns_cache_impl_test.cc | 3 +- .../alts/tsi_handshaker_test.cc | 3 +- test/mocks/event/mocks.cc | 2 +- test/server/connection_handler_test.cc | 3 +- test/server/overload_manager_impl_test.cc | 3 +- 13 files changed, 75 insertions(+), 54 deletions(-) diff --git a/test/common/router/rds_impl_test.cc b/test/common/router/rds_impl_test.cc index ac3a5c046fee..8438c1ce5414 100644 --- a/test/common/router/rds_impl_test.cc +++ b/test/common/router/rds_impl_test.cc @@ -363,7 +363,8 @@ TEST_F(RdsImplTest, VirtualHostUpdateWhenProviderHasBeenDeallocated) { parseHttpConnectionManagerFromYaml(rds_config), server_factory_context_, validation_visitor_, outer_init_manager_, "foo.", *route_config_provider_manager_); - EXPECT_CALL(server_factory_context_.dispatcher_, post(_)).WillOnce(SaveArg<0>(&post_cb)); + EXPECT_CALL(server_factory_context_.dispatcher_, post(testing::Matcher())) + .WillOnce(SaveArg<0>(&post_cb)); rds->requestVirtualHostsUpdate( "testing", local_thread_dispatcher, std::make_shared( diff --git a/test/common/router/scoped_rds_test.cc b/test/common/router/scoped_rds_test.cc index 7ef4868b25af..a5a3a267ce42 100644 --- a/test/common/router/scoped_rds_test.cc +++ b/test/common/router/scoped_rds_test.cc @@ -1063,7 +1063,7 @@ on_demand: true ScopeKeyPtr scope_key = getScopedRdsProvider()->config()->computeScopeKey( TestRequestHeaderMapImpl{{"Addr", "x-foo-key;x-bar-key"}}); - EXPECT_CALL(event_dispatcher_, post(_)); + EXPECT_CALL(event_dispatcher_, post(testing::Matcher())); std::function route_config_updated_cb = [](bool) {}; getScopedRdsProvider()->onDemandRdsUpdate(std::move(scope_key), event_dispatcher_, std::move(route_config_updated_cb)); @@ -1135,8 +1135,8 @@ on_demand: true "foo_routes"); ScopeKeyPtr scope_key = getScopedRdsProvider()->config()->computeScopeKey( TestRequestHeaderMapImpl{{"Addr", "x-foo-key;x-bar-key"}}); - EXPECT_CALL(server_factory_context_.dispatcher_, post(_)); - EXPECT_CALL(event_dispatcher_, post(_)); + EXPECT_CALL(server_factory_context_.dispatcher_, post(testing::Matcher())); + EXPECT_CALL(event_dispatcher_, post(testing::Matcher())); std::function route_config_updated_cb = [](bool) {}; getScopedRdsProvider()->onDemandRdsUpdate(std::move(scope_key), event_dispatcher_, std::move(route_config_updated_cb)); @@ -1270,11 +1270,11 @@ on_demand: true std::move(route_config_updated_cb)); } // After on demand request, push rds update, the callbacks will be executed. - EXPECT_CALL(event_dispatcher_, post(_)).Times(5); + EXPECT_CALL(event_dispatcher_, post(testing::Matcher())).Times(5); pushRdsConfig({"foo_routes"}, "111"); // Route table have been fetched, callbacks will be executed immediately. for (int i = 0; i < 5; i++) { - EXPECT_CALL(event_dispatcher_, post(_)); + EXPECT_CALL(event_dispatcher_, post(testing::Matcher())); ScopeKeyPtr scope_key = getScopedRdsProvider()->config()->computeScopeKey( TestRequestHeaderMapImpl{{"Addr", "x-foo-key;x-foo-key"}}); std::function route_config_updated_cb = [](bool) {}; @@ -1295,7 +1295,7 @@ TEST_F(ScopedRdsTest, DanglingSubscriptionOnDemandUpdate) { setup(); std::function route_config_updated_cb = [](bool) {}; Event::PostCb temp_post_cb; - EXPECT_CALL(server_factory_context_.dispatcher_, post(_)) + EXPECT_CALL(server_factory_context_.dispatcher_, post(testing::Matcher())) .WillOnce(testing::SaveArg<0>(&temp_post_cb)); std::shared_ptr scope_key = getScopedRdsProvider()->config()->computeScopeKey( @@ -1304,7 +1304,7 @@ TEST_F(ScopedRdsTest, DanglingSubscriptionOnDemandUpdate) { std::move(route_config_updated_cb)); // Destroy the scoped_rds subscription by destroying its only config provider. provider_.reset(); - EXPECT_CALL(event_dispatcher_, post(_)); + EXPECT_CALL(event_dispatcher_, post(testing::Matcher())); EXPECT_NO_THROW(temp_post_cb()); } @@ -1337,7 +1337,7 @@ on_demand: true std::move(route_config_updated_cb)); } // After on demand request, push rds update, the callbacks will be executed. - EXPECT_CALL(event_dispatcher_, post(_)); + EXPECT_CALL(event_dispatcher_, post(testing::Matcher())); pushRdsConfig({"foo_routes"}, "111"); ScopeKeyPtr scope_key = getScopedRdsProvider()->config()->computeScopeKey( @@ -1345,7 +1345,7 @@ on_demand: true // Delete the scope route. EXPECT_NO_THROW(srds_subscription_->onConfigUpdate({}, "2")); EXPECT_EQ(0UL, all_scopes_.value()); - EXPECT_CALL(event_dispatcher_, post(_)); + EXPECT_CALL(event_dispatcher_, post(testing::Matcher())); // Scope no longer exists after srds update. std::function route_config_updated_cb = [](bool scope_exist) { EXPECT_FALSE(scope_exist); diff --git a/test/common/stats/thread_local_store_test.cc b/test/common/stats/thread_local_store_test.cc index 6d8de94d0463..848084108244 100644 --- a/test/common/stats/thread_local_store_test.cc +++ b/test/common/stats/thread_local_store_test.cc @@ -498,7 +498,7 @@ TEST_F(StatsThreadLocalStoreTest, ScopeDelete) { CounterSharedPtr c1 = TestUtility::findCounter(*store_, "scope1.c1"); EXPECT_EQ("scope1.c1", c1->name()); - EXPECT_CALL(main_thread_dispatcher_, post(_)); + EXPECT_CALL(main_thread_dispatcher_, post(testing::Matcher())); EXPECT_CALL(tls_, runOnAllThreads(_, _)); scope1.reset(); EXPECT_EQ(0UL, store_->counters().size()); diff --git a/test/common/thread_local/thread_local_impl_test.cc b/test/common/thread_local/thread_local_impl_test.cc index 89a13a6263ef..642e3dbb36ee 100644 --- a/test/common/thread_local/thread_local_impl_test.cc +++ b/test/common/thread_local/thread_local_impl_test.cc @@ -42,7 +42,7 @@ class ThreadLocalInstanceImplTest : public testing::Test { ThreadLocalInstanceImplTest() { tls_.registerThread(main_dispatcher_, true); EXPECT_EQ(&main_dispatcher_, &tls_.dispatcher()); - EXPECT_CALL(thread_dispatcher_, post(_)); + EXPECT_CALL(thread_dispatcher_, post(testing::Matcher())); tls_.registerThread(thread_dispatcher_, false); } @@ -51,7 +51,7 @@ class ThreadLocalInstanceImplTest : public testing::Test { TestThreadLocalObject& setObject(TypedSlot<>& slot) { std::shared_ptr object(new TestThreadLocalObject()); TestThreadLocalObject& object_ref = *object; - EXPECT_CALL(thread_dispatcher_, post(_)); + EXPECT_CALL(thread_dispatcher_, post(testing::Matcher())); EXPECT_CALL(*this, createThreadLocal(Ref(thread_dispatcher_))).WillOnce(ReturnPointee(&object)); EXPECT_CALL(*this, createThreadLocal(Ref(main_dispatcher_))).WillOnce(ReturnPointee(&object)); slot.set([this](Event::Dispatcher& dispatcher) -> ThreadLocalObjectSharedPtr { @@ -71,7 +71,7 @@ TEST_F(ThreadLocalInstanceImplTest, All) { InSequence s; // Free a slot without ever calling set. - EXPECT_CALL(thread_dispatcher_, post(_)); + EXPECT_CALL(thread_dispatcher_, post(testing::Matcher())); TypedSlotPtr<> slot1 = TypedSlot<>::makeUnique(tls_); slot1.reset(); EXPECT_EQ(freeSlotIndexesListSize(), 1); @@ -82,7 +82,7 @@ TEST_F(ThreadLocalInstanceImplTest, All) { TestThreadLocalObject& object_ref2 = setObject(*slot2); EXPECT_EQ(freeSlotIndexesListSize(), 0); - EXPECT_CALL(thread_dispatcher_, post(_)); + EXPECT_CALL(thread_dispatcher_, post(testing::Matcher())); EXPECT_CALL(object_ref2, onDestroy()); EXPECT_EQ(freeSlotIndexesListSize(), 0); slot2.reset(); @@ -116,10 +116,12 @@ struct ThreadStatus { class CallbackNotInvokedAfterDeletionTest : public ThreadLocalInstanceImplTest { protected: CallbackNotInvokedAfterDeletionTest() : slot_(TypedSlot<>::makeUnique(tls_)) { - EXPECT_CALL(thread_dispatcher_, post(_)).Times(4).WillRepeatedly(Invoke([&](Event::PostCb cb) { - // Holds the posted callback. - holder_.push_back(cb); - })); + EXPECT_CALL(thread_dispatcher_, post(testing::Matcher())) + .Times(4) + .WillRepeatedly(Invoke([&](Event::PostCb cb) { + // Holds the posted callback. + holder_.push_back(cb); + })); slot_->set([this](Event::Dispatcher&) { // Callbacks happen on the main thread but not the workers, so track the total. @@ -134,7 +136,7 @@ class CallbackNotInvokedAfterDeletionTest : public ThreadLocalInstanceImplTest { slot_.reset(); EXPECT_EQ(freeSlotIndexesListSize(), 1); - EXPECT_CALL(main_dispatcher_, post(_)); + EXPECT_CALL(main_dispatcher_, post(testing::Matcher())); while (!holder_.empty()) { holder_.front()(); holder_.pop_front(); @@ -182,7 +184,7 @@ TEST_F(ThreadLocalInstanceImplTest, UpdateCallback) { TestThreadLocalObject& object_ref = setObject(slot); auto update_cb = [&update_called](OptRef) { ++update_called; }; - EXPECT_CALL(thread_dispatcher_, post(_)); + EXPECT_CALL(thread_dispatcher_, post(testing::Matcher())); EXPECT_CALL(object_ref, onDestroy()); slot.runOnAllThreads(update_cb); @@ -201,7 +203,7 @@ TEST_F(ThreadLocalInstanceImplTest, TypedUpdateCallback) { TypedSlot slot(tls_); uint32_t update_called = 0; - EXPECT_CALL(thread_dispatcher_, post(_)); + EXPECT_CALL(thread_dispatcher_, post(testing::Matcher())); slot.set([](Event::Dispatcher&) -> std::shared_ptr { auto s = std::make_shared(); s->str_ = "hello"; @@ -214,7 +216,7 @@ TEST_F(ThreadLocalInstanceImplTest, TypedUpdateCallback) { EXPECT_TRUE(s.has_value()); s->str_ = "goodbye"; }; - EXPECT_CALL(thread_dispatcher_, post(_)); + EXPECT_CALL(thread_dispatcher_, post(testing::Matcher())); slot.runOnAllThreads(update_cb); // Tests a few different ways of getting at the slot data. @@ -232,7 +234,7 @@ TEST_F(ThreadLocalInstanceImplTest, NoDataCallback) { TypedSlot slot(tls_); uint32_t update_called = 0; - EXPECT_CALL(thread_dispatcher_, post(_)); + EXPECT_CALL(thread_dispatcher_, post(testing::Matcher())); slot.set([](Event::Dispatcher&) -> std::shared_ptr { return nullptr; }); EXPECT_FALSE(slot.get().has_value()); @@ -240,7 +242,7 @@ TEST_F(ThreadLocalInstanceImplTest, NoDataCallback) { ++update_called; EXPECT_FALSE(s.has_value()); }; - EXPECT_CALL(thread_dispatcher_, post(_)); + EXPECT_CALL(thread_dispatcher_, post(testing::Matcher())); slot.runOnAllThreads(update_cb); EXPECT_FALSE(slot.get().has_value()); @@ -258,8 +260,8 @@ TEST_F(ThreadLocalInstanceImplTest, RunOnAllThreads) { TypedSlotPtr<> tlsptr = TypedSlot<>::makeUnique(tls_); TestThreadLocalObject& object_ref = setObject(*tlsptr); - EXPECT_CALL(thread_dispatcher_, post(_)); - EXPECT_CALL(main_dispatcher_, post(_)); + EXPECT_CALL(thread_dispatcher_, post(testing::Matcher())); + EXPECT_CALL(main_dispatcher_, post(testing::Matcher())); // Ensure that the thread local call back and all_thread_complete call back are called. ThreadStatus thread_status; diff --git a/test/common/upstream/cluster_manager_impl_test.cc b/test/common/upstream/cluster_manager_impl_test.cc index 094bf420c664..d53655e3db7a 100644 --- a/test/common/upstream/cluster_manager_impl_test.cc +++ b/test/common/upstream/cluster_manager_impl_test.cc @@ -874,7 +874,7 @@ TEST_F(ClusterManagerImplTest, HttpHealthChecker) { createClientConnection_( PointeesEq(Network::Utility::resolveUrl("tcp://127.0.0.1:11001")), _, _, _)) .WillOnce(Return(connection)); - EXPECT_CALL(factory_.dispatcher_, post(_)); + EXPECT_CALL(factory_.dispatcher_, post(testing::Matcher())); create(parseBootstrapFromV3Yaml(yaml)); factory_.tls_.shutdownThread(); factory_.dispatcher_.to_delete_.clear(); diff --git a/test/common/upstream/health_checker_impl_test.cc b/test/common/upstream/health_checker_impl_test.cc index e57fe6104857..f646bcef8d59 100644 --- a/test/common/upstream/health_checker_impl_test.cc +++ b/test/common/upstream/health_checker_impl_test.cc @@ -3840,7 +3840,8 @@ TEST_F(TcpHealthCheckerImplTest, PassiveFailureCrossThreadRemoveHostRace) { // Do a passive failure. This will not reset the active HC timers. Event::PostCb post_cb; - EXPECT_CALL(dispatcher_, post(_)).WillOnce(SaveArg<0>(&post_cb)); + EXPECT_CALL(dispatcher_, post(testing::Matcher())) + .WillOnce(SaveArg<0>(&post_cb)); cluster_->prioritySet().getMockHostSet(0)->hosts_[0]->healthChecker().setUnhealthy( HealthCheckHostMonitor::UnhealthyType::ImmediateHealthCheckFail); @@ -3870,7 +3871,8 @@ TEST_F(TcpHealthCheckerImplTest, PassiveFailureCrossThreadRemoveClusterRace) { // Do a passive failure. This will not reset the active HC timers. Event::PostCb post_cb; - EXPECT_CALL(dispatcher_, post(_)).WillOnce(SaveArg<0>(&post_cb)); + EXPECT_CALL(dispatcher_, post(testing::Matcher())) + .WillOnce(SaveArg<0>(&post_cb)); cluster_->prioritySet().getMockHostSet(0)->hosts_[0]->healthChecker().setUnhealthy( HealthCheckHostMonitor::UnhealthyType::ImmediateHealthCheckFail); diff --git a/test/common/upstream/original_dst_cluster_test.cc b/test/common/upstream/original_dst_cluster_test.cc index 9ecd87ede145..653da5d5e04e 100644 --- a/test/common/upstream/original_dst_cluster_test.cc +++ b/test/common/upstream/original_dst_cluster_test.cc @@ -205,7 +205,7 @@ TEST_F(OriginalDstClusterTest, NoContext) { { TestLoadBalancerContext lb_context(nullptr); OriginalDstCluster::LoadBalancer lb(cluster_); - EXPECT_CALL(dispatcher_, post(_)).Times(0); + EXPECT_CALL(dispatcher_, post(testing::Matcher())).Times(0); HostConstSharedPtr host = lb.chooseHost(&lb_context); EXPECT_EQ(host, nullptr); } @@ -219,7 +219,7 @@ TEST_F(OriginalDstClusterTest, NoContext) { // tests we do not have the thread local clusters, so we pass a reference to the HostSet of the // primary cluster. The implementation handles both cases the same. OriginalDstCluster::LoadBalancer lb(cluster_); - EXPECT_CALL(dispatcher_, post(_)).Times(0); + EXPECT_CALL(dispatcher_, post(testing::Matcher())).Times(0); HostConstSharedPtr host = lb.chooseHost(&lb_context); EXPECT_EQ(host, nullptr); } @@ -232,7 +232,7 @@ TEST_F(OriginalDstClusterTest, NoContext) { std::make_shared("unix://foo")); OriginalDstCluster::LoadBalancer lb(cluster_); - EXPECT_CALL(dispatcher_, post(_)).Times(0); + EXPECT_CALL(dispatcher_, post(testing::Matcher())).Times(0); HostConstSharedPtr host = lb.chooseHost(&lb_context); EXPECT_EQ(host, nullptr); } @@ -267,7 +267,8 @@ TEST_F(OriginalDstClusterTest, Membership) { std::make_shared("10.10.11.11")); Event::PostCb post_cb; - EXPECT_CALL(dispatcher_, post(_)).WillOnce(SaveArg<0>(&post_cb)); + EXPECT_CALL(dispatcher_, post(testing::Matcher())) + .WillOnce(SaveArg<0>(&post_cb)); // Mock the cluster manager by recreating the load balancer each time to get a fresh host map HostConstSharedPtr host = OriginalDstCluster::LoadBalancer(cluster_).chooseHost(&lb_context); post_cb(); @@ -316,7 +317,8 @@ TEST_F(OriginalDstClusterTest, Membership) { // New host gets created EXPECT_CALL(membership_updated_, ready()); - EXPECT_CALL(dispatcher_, post(_)).WillOnce(SaveArg<0>(&post_cb)); + EXPECT_CALL(dispatcher_, post(testing::Matcher())) + .WillOnce(SaveArg<0>(&post_cb)); // Mock the cluster manager by recreating the load balancer with the new host map HostConstSharedPtr host3 = OriginalDstCluster::LoadBalancer(cluster_).chooseHost(&lb_context); post_cb(); @@ -363,14 +365,16 @@ TEST_F(OriginalDstClusterTest, Membership2) { OriginalDstCluster::LoadBalancer lb(cluster_); EXPECT_CALL(membership_updated_, ready()); Event::PostCb post_cb; - EXPECT_CALL(dispatcher_, post(_)).WillOnce(SaveArg<0>(&post_cb)); + EXPECT_CALL(dispatcher_, post(testing::Matcher())) + .WillOnce(SaveArg<0>(&post_cb)); HostConstSharedPtr host1 = lb.chooseHost(&lb_context1); post_cb(); ASSERT_NE(host1, nullptr); EXPECT_EQ(*connection1.addressProvider().localAddress(), *host1->address()); EXPECT_CALL(membership_updated_, ready()); - EXPECT_CALL(dispatcher_, post(_)).WillOnce(SaveArg<0>(&post_cb)); + EXPECT_CALL(dispatcher_, post(testing::Matcher())) + .WillOnce(SaveArg<0>(&post_cb)); HostConstSharedPtr host2 = lb.chooseHost(&lb_context2); post_cb(); ASSERT_NE(host2, nullptr); @@ -446,7 +450,8 @@ TEST_F(OriginalDstClusterTest, Connection) { OriginalDstCluster::LoadBalancer lb(cluster_); Event::PostCb post_cb; - EXPECT_CALL(dispatcher_, post(_)).WillOnce(SaveArg<0>(&post_cb)); + EXPECT_CALL(dispatcher_, post(testing::Matcher())) + .WillOnce(SaveArg<0>(&post_cb)); HostConstSharedPtr host = lb.chooseHost(&lb_context); post_cb(); ASSERT_NE(host, nullptr); @@ -496,7 +501,8 @@ TEST_F(OriginalDstClusterTest, MultipleClusters) { OriginalDstCluster::LoadBalancer lb(cluster_); Event::PostCb post_cb; - EXPECT_CALL(dispatcher_, post(_)).WillOnce(SaveArg<0>(&post_cb)); + EXPECT_CALL(dispatcher_, post(testing::Matcher())) + .WillOnce(SaveArg<0>(&post_cb)); HostConstSharedPtr host = lb.chooseHost(&lb_context); post_cb(); ASSERT_NE(host, nullptr); @@ -539,7 +545,8 @@ TEST_F(OriginalDstClusterTest, UseHttpHeaderEnabled) { "127.0.0.1:5555"); EXPECT_CALL(membership_updated_, ready()); - EXPECT_CALL(dispatcher_, post(_)).WillOnce(SaveArg<0>(&post_cb)); + EXPECT_CALL(dispatcher_, post(testing::Matcher())) + .WillOnce(SaveArg<0>(&post_cb)); HostConstSharedPtr host1 = lb.chooseHost(&lb_context1); post_cb(); ASSERT_NE(host1, nullptr); @@ -553,7 +560,8 @@ TEST_F(OriginalDstClusterTest, UseHttpHeaderEnabled) { "127.0.0.1:5556"); EXPECT_CALL(membership_updated_, ready()); - EXPECT_CALL(dispatcher_, post(_)).WillOnce(SaveArg<0>(&post_cb)); + EXPECT_CALL(dispatcher_, post(testing::Matcher())) + .WillOnce(SaveArg<0>(&post_cb)); HostConstSharedPtr host2 = lb.chooseHost(&lb_context2); post_cb(); ASSERT_NE(host2, nullptr); @@ -563,7 +571,7 @@ TEST_F(OriginalDstClusterTest, UseHttpHeaderEnabled) { TestLoadBalancerContext lb_context3(nullptr, Http::Headers::get().EnvoyOriginalDstHost.get(), ""); EXPECT_CALL(membership_updated_, ready()).Times(0); - EXPECT_CALL(dispatcher_, post(_)).Times(0); + EXPECT_CALL(dispatcher_, post(testing::Matcher())).Times(0); HostConstSharedPtr host3 = lb.chooseHost(&lb_context3); EXPECT_EQ(host3, nullptr); EXPECT_EQ( @@ -574,7 +582,7 @@ TEST_F(OriginalDstClusterTest, UseHttpHeaderEnabled) { "a.b.c.d"); EXPECT_CALL(membership_updated_, ready()).Times(0); - EXPECT_CALL(dispatcher_, post(_)).Times(0); + EXPECT_CALL(dispatcher_, post(testing::Matcher())).Times(0); HostConstSharedPtr host4 = lb.chooseHost(&lb_context4); EXPECT_EQ(host4, nullptr); EXPECT_EQ( @@ -611,7 +619,8 @@ TEST_F(OriginalDstClusterTest, UseHttpHeaderDisabled) { "127.0.0.1:5555"); EXPECT_CALL(membership_updated_, ready()); - EXPECT_CALL(dispatcher_, post(_)).WillOnce(SaveArg<0>(&post_cb)); + EXPECT_CALL(dispatcher_, post(testing::Matcher())) + .WillOnce(SaveArg<0>(&post_cb)); HostConstSharedPtr host1 = lb.chooseHost(&lb_context1); post_cb(); ASSERT_NE(host1, nullptr); @@ -625,7 +634,7 @@ TEST_F(OriginalDstClusterTest, UseHttpHeaderDisabled) { "127.0.0.1:5555"); EXPECT_CALL(membership_updated_, ready()).Times(0); - EXPECT_CALL(dispatcher_, post(_)).Times(0); + EXPECT_CALL(dispatcher_, post(testing::Matcher())).Times(0); HostConstSharedPtr host2 = lb.chooseHost(&lb_context2); EXPECT_EQ(host2, nullptr); @@ -637,7 +646,7 @@ TEST_F(OriginalDstClusterTest, UseHttpHeaderDisabled) { "127.0.0.1:5555"); EXPECT_CALL(membership_updated_, ready()).Times(0); - EXPECT_CALL(dispatcher_, post(_)).Times(0); + EXPECT_CALL(dispatcher_, post(testing::Matcher())).Times(0); HostConstSharedPtr host3 = lb.chooseHost(&lb_context3); EXPECT_EQ(host3, nullptr); } diff --git a/test/common/upstream/outlier_detection_impl_test.cc b/test/common/upstream/outlier_detection_impl_test.cc index 1a985ddae542..c4ca1f45cd41 100644 --- a/test/common/upstream/outlier_detection_impl_test.cc +++ b/test/common/upstream/outlier_detection_impl_test.cc @@ -1437,7 +1437,8 @@ TEST_F(OutlierDetectorImplTest, CrossThreadRemoveRace) { loadRq(hosts_[0], 4, 500); Event::PostCb post_cb; - EXPECT_CALL(dispatcher_, post(_)).WillOnce(SaveArg<0>(&post_cb)); + EXPECT_CALL(dispatcher_, post(testing::Matcher())) + .WillOnce(SaveArg<0>(&post_cb)); loadRq(hosts_[0], 1, 500); // Remove before the cross thread event comes in. @@ -1459,7 +1460,8 @@ TEST_F(OutlierDetectorImplTest, CrossThreadDestroyRace) { loadRq(hosts_[0], 4, 500); Event::PostCb post_cb; - EXPECT_CALL(dispatcher_, post(_)).WillOnce(SaveArg<0>(&post_cb)); + EXPECT_CALL(dispatcher_, post(testing::Matcher())) + .WillOnce(SaveArg<0>(&post_cb)); loadRq(hosts_[0], 1, 500); // Destroy before the cross thread event comes in. @@ -1482,7 +1484,8 @@ TEST_F(OutlierDetectorImplTest, CrossThreadFailRace) { loadRq(hosts_[0], 4, 500); Event::PostCb post_cb; - EXPECT_CALL(dispatcher_, post(_)).WillOnce(SaveArg<0>(&post_cb)); + EXPECT_CALL(dispatcher_, post(testing::Matcher())) + .WillOnce(SaveArg<0>(&post_cb)); loadRq(hosts_[0], 1, 500); time_system_.setMonotonicTime(std::chrono::milliseconds(0)); diff --git a/test/extensions/common/dynamic_forward_proxy/dns_cache_impl_test.cc b/test/extensions/common/dynamic_forward_proxy/dns_cache_impl_test.cc index 5eb41286c467..6d58d66b9be5 100644 --- a/test/extensions/common/dynamic_forward_proxy/dns_cache_impl_test.cc +++ b/test/extensions/common/dynamic_forward_proxy/dns_cache_impl_test.cc @@ -347,7 +347,8 @@ TEST_F(DnsCacheImplTest, InlineResolve) { MockLoadDnsCacheEntryCallbacks callbacks; Event::PostCb post_cb; - EXPECT_CALL(dispatcher_, post(_)).WillOnce(SaveArg<0>(&post_cb)); + EXPECT_CALL(dispatcher_, post(testing::Matcher())) + .WillOnce(SaveArg<0>(&post_cb)); auto result = dns_cache_->loadDnsCacheEntry("localhost", 80, callbacks); EXPECT_EQ(DnsCache::LoadDnsCacheEntryStatus::Loading, result.status_); EXPECT_NE(result.handle_, nullptr); diff --git a/test/extensions/transport_sockets/alts/tsi_handshaker_test.cc b/test/extensions/transport_sockets/alts/tsi_handshaker_test.cc index 0b1afa2e0acc..c8e66f0876ca 100644 --- a/test/extensions/transport_sockets/alts/tsi_handshaker_test.cc +++ b/test/extensions/transport_sockets/alts/tsi_handshaker_test.cc @@ -152,7 +152,8 @@ TEST_F(TsiHandshakerTest, DeleteOnDone) { Buffer::OwnedImpl empty; std::function done; - EXPECT_CALL(dispatcher_, post(_)).WillOnce(SaveArg<0>(&done)); + EXPECT_CALL(dispatcher_, post(testing::Matcher())) + .WillOnce(SaveArg<0>(&done)); handshaker->next(empty); handshaker->deferredDelete(); diff --git a/test/mocks/event/mocks.cc b/test/mocks/event/mocks.cc index fae17cebddc4..9a131cf4cf8c 100644 --- a/test/mocks/event/mocks.cc +++ b/test/mocks/event/mocks.cc @@ -27,7 +27,7 @@ MockDispatcher::MockDispatcher(const std::string& name) : name_(name) { ON_CALL(*this, createScaledTimer_(_, _)).WillByDefault(ReturnNew>()); ON_CALL(*this, createScaledTypedTimer_(_, _)) .WillByDefault(ReturnNew>()); - ON_CALL(*this, post(testing::Matcher())) + ON_CALL(*this, post(testing::Matcher())) .WillByDefault(Invoke([](const PostCb& cb) -> void { cb(); })); ON_CALL(*this, post(testing::Matcher())).WillByDefault(Invoke([](PostCb&& cb) -> void { cb(); diff --git a/test/server/connection_handler_test.cc b/test/server/connection_handler_test.cc index dd55fea07a17..d273665916eb 100644 --- a/test/server/connection_handler_test.cc +++ b/test/server/connection_handler_test.cc @@ -285,7 +285,8 @@ TEST_F(ConnectionHandlerTest, RemoveListenerDuringRebalance) { // Fake a balancer posting a connection to us. Event::PostCb post_cb; - EXPECT_CALL(dispatcher_, post(_)).WillOnce(SaveArg<0>(&post_cb)); + EXPECT_CALL(dispatcher_, post(testing::Matcher())) + .WillOnce(SaveArg<0>(&post_cb)); Network::MockConnectionSocket* connection = new NiceMock(); current_handler->incNumConnections(); #ifndef NDEBUG diff --git a/test/server/overload_manager_impl_test.cc b/test/server/overload_manager_impl_test.cc index 09d58bfa89e0..3d9411cd88f7 100644 --- a/test/server/overload_manager_impl_test.cc +++ b/test/server/overload_manager_impl_test.cc @@ -543,7 +543,8 @@ TEST_F(OverloadManagerImplTest, AdjustScaleFactor) { manager->start(); - EXPECT_CALL(mock_dispatcher, post).WillOnce(InvokeArgument<0>()); + EXPECT_CALL(mock_dispatcher, post(testing::Matcher())) + .WillOnce(InvokeArgument<0>()); // The scaled trigger has range [0.5, 1.0] so 0.6 should map to a scale value of 0.2, which means // a timer scale factor of 0.8 (1 - 0.2). EXPECT_CALL(*mock_scaled_timer_manager, From 16e1aca3e60a9a913fd055fb6d8b1856f37caa50 Mon Sep 17 00:00:00 2001 From: Yuchen Dai Date: Wed, 10 Feb 2021 00:08:01 -0800 Subject: [PATCH 23/40] Revert "fix ambigious" This reverts commit 7782047509bd7abf94d4a7f0642eac0c4adec239. Signed-off-by: Yuchen Dai --- test/common/router/rds_impl_test.cc | 3 +- test/common/router/scoped_rds_test.cc | 18 ++++---- test/common/stats/thread_local_store_test.cc | 2 +- .../thread_local/thread_local_impl_test.cc | 34 ++++++++------- .../upstream/cluster_manager_impl_test.cc | 2 +- .../upstream/health_checker_impl_test.cc | 6 +-- .../upstream/original_dst_cluster_test.cc | 41 ++++++++----------- .../upstream/outlier_detection_impl_test.cc | 9 ++-- .../dns_cache_impl_test.cc | 3 +- .../alts/tsi_handshaker_test.cc | 3 +- test/mocks/event/mocks.cc | 2 +- test/server/connection_handler_test.cc | 3 +- test/server/overload_manager_impl_test.cc | 3 +- 13 files changed, 54 insertions(+), 75 deletions(-) diff --git a/test/common/router/rds_impl_test.cc b/test/common/router/rds_impl_test.cc index 8438c1ce5414..ac3a5c046fee 100644 --- a/test/common/router/rds_impl_test.cc +++ b/test/common/router/rds_impl_test.cc @@ -363,8 +363,7 @@ TEST_F(RdsImplTest, VirtualHostUpdateWhenProviderHasBeenDeallocated) { parseHttpConnectionManagerFromYaml(rds_config), server_factory_context_, validation_visitor_, outer_init_manager_, "foo.", *route_config_provider_manager_); - EXPECT_CALL(server_factory_context_.dispatcher_, post(testing::Matcher())) - .WillOnce(SaveArg<0>(&post_cb)); + EXPECT_CALL(server_factory_context_.dispatcher_, post(_)).WillOnce(SaveArg<0>(&post_cb)); rds->requestVirtualHostsUpdate( "testing", local_thread_dispatcher, std::make_shared( diff --git a/test/common/router/scoped_rds_test.cc b/test/common/router/scoped_rds_test.cc index a5a3a267ce42..7ef4868b25af 100644 --- a/test/common/router/scoped_rds_test.cc +++ b/test/common/router/scoped_rds_test.cc @@ -1063,7 +1063,7 @@ on_demand: true ScopeKeyPtr scope_key = getScopedRdsProvider()->config()->computeScopeKey( TestRequestHeaderMapImpl{{"Addr", "x-foo-key;x-bar-key"}}); - EXPECT_CALL(event_dispatcher_, post(testing::Matcher())); + EXPECT_CALL(event_dispatcher_, post(_)); std::function route_config_updated_cb = [](bool) {}; getScopedRdsProvider()->onDemandRdsUpdate(std::move(scope_key), event_dispatcher_, std::move(route_config_updated_cb)); @@ -1135,8 +1135,8 @@ on_demand: true "foo_routes"); ScopeKeyPtr scope_key = getScopedRdsProvider()->config()->computeScopeKey( TestRequestHeaderMapImpl{{"Addr", "x-foo-key;x-bar-key"}}); - EXPECT_CALL(server_factory_context_.dispatcher_, post(testing::Matcher())); - EXPECT_CALL(event_dispatcher_, post(testing::Matcher())); + EXPECT_CALL(server_factory_context_.dispatcher_, post(_)); + EXPECT_CALL(event_dispatcher_, post(_)); std::function route_config_updated_cb = [](bool) {}; getScopedRdsProvider()->onDemandRdsUpdate(std::move(scope_key), event_dispatcher_, std::move(route_config_updated_cb)); @@ -1270,11 +1270,11 @@ on_demand: true std::move(route_config_updated_cb)); } // After on demand request, push rds update, the callbacks will be executed. - EXPECT_CALL(event_dispatcher_, post(testing::Matcher())).Times(5); + EXPECT_CALL(event_dispatcher_, post(_)).Times(5); pushRdsConfig({"foo_routes"}, "111"); // Route table have been fetched, callbacks will be executed immediately. for (int i = 0; i < 5; i++) { - EXPECT_CALL(event_dispatcher_, post(testing::Matcher())); + EXPECT_CALL(event_dispatcher_, post(_)); ScopeKeyPtr scope_key = getScopedRdsProvider()->config()->computeScopeKey( TestRequestHeaderMapImpl{{"Addr", "x-foo-key;x-foo-key"}}); std::function route_config_updated_cb = [](bool) {}; @@ -1295,7 +1295,7 @@ TEST_F(ScopedRdsTest, DanglingSubscriptionOnDemandUpdate) { setup(); std::function route_config_updated_cb = [](bool) {}; Event::PostCb temp_post_cb; - EXPECT_CALL(server_factory_context_.dispatcher_, post(testing::Matcher())) + EXPECT_CALL(server_factory_context_.dispatcher_, post(_)) .WillOnce(testing::SaveArg<0>(&temp_post_cb)); std::shared_ptr scope_key = getScopedRdsProvider()->config()->computeScopeKey( @@ -1304,7 +1304,7 @@ TEST_F(ScopedRdsTest, DanglingSubscriptionOnDemandUpdate) { std::move(route_config_updated_cb)); // Destroy the scoped_rds subscription by destroying its only config provider. provider_.reset(); - EXPECT_CALL(event_dispatcher_, post(testing::Matcher())); + EXPECT_CALL(event_dispatcher_, post(_)); EXPECT_NO_THROW(temp_post_cb()); } @@ -1337,7 +1337,7 @@ on_demand: true std::move(route_config_updated_cb)); } // After on demand request, push rds update, the callbacks will be executed. - EXPECT_CALL(event_dispatcher_, post(testing::Matcher())); + EXPECT_CALL(event_dispatcher_, post(_)); pushRdsConfig({"foo_routes"}, "111"); ScopeKeyPtr scope_key = getScopedRdsProvider()->config()->computeScopeKey( @@ -1345,7 +1345,7 @@ on_demand: true // Delete the scope route. EXPECT_NO_THROW(srds_subscription_->onConfigUpdate({}, "2")); EXPECT_EQ(0UL, all_scopes_.value()); - EXPECT_CALL(event_dispatcher_, post(testing::Matcher())); + EXPECT_CALL(event_dispatcher_, post(_)); // Scope no longer exists after srds update. std::function route_config_updated_cb = [](bool scope_exist) { EXPECT_FALSE(scope_exist); diff --git a/test/common/stats/thread_local_store_test.cc b/test/common/stats/thread_local_store_test.cc index 848084108244..6d8de94d0463 100644 --- a/test/common/stats/thread_local_store_test.cc +++ b/test/common/stats/thread_local_store_test.cc @@ -498,7 +498,7 @@ TEST_F(StatsThreadLocalStoreTest, ScopeDelete) { CounterSharedPtr c1 = TestUtility::findCounter(*store_, "scope1.c1"); EXPECT_EQ("scope1.c1", c1->name()); - EXPECT_CALL(main_thread_dispatcher_, post(testing::Matcher())); + EXPECT_CALL(main_thread_dispatcher_, post(_)); EXPECT_CALL(tls_, runOnAllThreads(_, _)); scope1.reset(); EXPECT_EQ(0UL, store_->counters().size()); diff --git a/test/common/thread_local/thread_local_impl_test.cc b/test/common/thread_local/thread_local_impl_test.cc index 642e3dbb36ee..89a13a6263ef 100644 --- a/test/common/thread_local/thread_local_impl_test.cc +++ b/test/common/thread_local/thread_local_impl_test.cc @@ -42,7 +42,7 @@ class ThreadLocalInstanceImplTest : public testing::Test { ThreadLocalInstanceImplTest() { tls_.registerThread(main_dispatcher_, true); EXPECT_EQ(&main_dispatcher_, &tls_.dispatcher()); - EXPECT_CALL(thread_dispatcher_, post(testing::Matcher())); + EXPECT_CALL(thread_dispatcher_, post(_)); tls_.registerThread(thread_dispatcher_, false); } @@ -51,7 +51,7 @@ class ThreadLocalInstanceImplTest : public testing::Test { TestThreadLocalObject& setObject(TypedSlot<>& slot) { std::shared_ptr object(new TestThreadLocalObject()); TestThreadLocalObject& object_ref = *object; - EXPECT_CALL(thread_dispatcher_, post(testing::Matcher())); + EXPECT_CALL(thread_dispatcher_, post(_)); EXPECT_CALL(*this, createThreadLocal(Ref(thread_dispatcher_))).WillOnce(ReturnPointee(&object)); EXPECT_CALL(*this, createThreadLocal(Ref(main_dispatcher_))).WillOnce(ReturnPointee(&object)); slot.set([this](Event::Dispatcher& dispatcher) -> ThreadLocalObjectSharedPtr { @@ -71,7 +71,7 @@ TEST_F(ThreadLocalInstanceImplTest, All) { InSequence s; // Free a slot without ever calling set. - EXPECT_CALL(thread_dispatcher_, post(testing::Matcher())); + EXPECT_CALL(thread_dispatcher_, post(_)); TypedSlotPtr<> slot1 = TypedSlot<>::makeUnique(tls_); slot1.reset(); EXPECT_EQ(freeSlotIndexesListSize(), 1); @@ -82,7 +82,7 @@ TEST_F(ThreadLocalInstanceImplTest, All) { TestThreadLocalObject& object_ref2 = setObject(*slot2); EXPECT_EQ(freeSlotIndexesListSize(), 0); - EXPECT_CALL(thread_dispatcher_, post(testing::Matcher())); + EXPECT_CALL(thread_dispatcher_, post(_)); EXPECT_CALL(object_ref2, onDestroy()); EXPECT_EQ(freeSlotIndexesListSize(), 0); slot2.reset(); @@ -116,12 +116,10 @@ struct ThreadStatus { class CallbackNotInvokedAfterDeletionTest : public ThreadLocalInstanceImplTest { protected: CallbackNotInvokedAfterDeletionTest() : slot_(TypedSlot<>::makeUnique(tls_)) { - EXPECT_CALL(thread_dispatcher_, post(testing::Matcher())) - .Times(4) - .WillRepeatedly(Invoke([&](Event::PostCb cb) { - // Holds the posted callback. - holder_.push_back(cb); - })); + EXPECT_CALL(thread_dispatcher_, post(_)).Times(4).WillRepeatedly(Invoke([&](Event::PostCb cb) { + // Holds the posted callback. + holder_.push_back(cb); + })); slot_->set([this](Event::Dispatcher&) { // Callbacks happen on the main thread but not the workers, so track the total. @@ -136,7 +134,7 @@ class CallbackNotInvokedAfterDeletionTest : public ThreadLocalInstanceImplTest { slot_.reset(); EXPECT_EQ(freeSlotIndexesListSize(), 1); - EXPECT_CALL(main_dispatcher_, post(testing::Matcher())); + EXPECT_CALL(main_dispatcher_, post(_)); while (!holder_.empty()) { holder_.front()(); holder_.pop_front(); @@ -184,7 +182,7 @@ TEST_F(ThreadLocalInstanceImplTest, UpdateCallback) { TestThreadLocalObject& object_ref = setObject(slot); auto update_cb = [&update_called](OptRef) { ++update_called; }; - EXPECT_CALL(thread_dispatcher_, post(testing::Matcher())); + EXPECT_CALL(thread_dispatcher_, post(_)); EXPECT_CALL(object_ref, onDestroy()); slot.runOnAllThreads(update_cb); @@ -203,7 +201,7 @@ TEST_F(ThreadLocalInstanceImplTest, TypedUpdateCallback) { TypedSlot slot(tls_); uint32_t update_called = 0; - EXPECT_CALL(thread_dispatcher_, post(testing::Matcher())); + EXPECT_CALL(thread_dispatcher_, post(_)); slot.set([](Event::Dispatcher&) -> std::shared_ptr { auto s = std::make_shared(); s->str_ = "hello"; @@ -216,7 +214,7 @@ TEST_F(ThreadLocalInstanceImplTest, TypedUpdateCallback) { EXPECT_TRUE(s.has_value()); s->str_ = "goodbye"; }; - EXPECT_CALL(thread_dispatcher_, post(testing::Matcher())); + EXPECT_CALL(thread_dispatcher_, post(_)); slot.runOnAllThreads(update_cb); // Tests a few different ways of getting at the slot data. @@ -234,7 +232,7 @@ TEST_F(ThreadLocalInstanceImplTest, NoDataCallback) { TypedSlot slot(tls_); uint32_t update_called = 0; - EXPECT_CALL(thread_dispatcher_, post(testing::Matcher())); + EXPECT_CALL(thread_dispatcher_, post(_)); slot.set([](Event::Dispatcher&) -> std::shared_ptr { return nullptr; }); EXPECT_FALSE(slot.get().has_value()); @@ -242,7 +240,7 @@ TEST_F(ThreadLocalInstanceImplTest, NoDataCallback) { ++update_called; EXPECT_FALSE(s.has_value()); }; - EXPECT_CALL(thread_dispatcher_, post(testing::Matcher())); + EXPECT_CALL(thread_dispatcher_, post(_)); slot.runOnAllThreads(update_cb); EXPECT_FALSE(slot.get().has_value()); @@ -260,8 +258,8 @@ TEST_F(ThreadLocalInstanceImplTest, RunOnAllThreads) { TypedSlotPtr<> tlsptr = TypedSlot<>::makeUnique(tls_); TestThreadLocalObject& object_ref = setObject(*tlsptr); - EXPECT_CALL(thread_dispatcher_, post(testing::Matcher())); - EXPECT_CALL(main_dispatcher_, post(testing::Matcher())); + EXPECT_CALL(thread_dispatcher_, post(_)); + EXPECT_CALL(main_dispatcher_, post(_)); // Ensure that the thread local call back and all_thread_complete call back are called. ThreadStatus thread_status; diff --git a/test/common/upstream/cluster_manager_impl_test.cc b/test/common/upstream/cluster_manager_impl_test.cc index d53655e3db7a..094bf420c664 100644 --- a/test/common/upstream/cluster_manager_impl_test.cc +++ b/test/common/upstream/cluster_manager_impl_test.cc @@ -874,7 +874,7 @@ TEST_F(ClusterManagerImplTest, HttpHealthChecker) { createClientConnection_( PointeesEq(Network::Utility::resolveUrl("tcp://127.0.0.1:11001")), _, _, _)) .WillOnce(Return(connection)); - EXPECT_CALL(factory_.dispatcher_, post(testing::Matcher())); + EXPECT_CALL(factory_.dispatcher_, post(_)); create(parseBootstrapFromV3Yaml(yaml)); factory_.tls_.shutdownThread(); factory_.dispatcher_.to_delete_.clear(); diff --git a/test/common/upstream/health_checker_impl_test.cc b/test/common/upstream/health_checker_impl_test.cc index f646bcef8d59..e57fe6104857 100644 --- a/test/common/upstream/health_checker_impl_test.cc +++ b/test/common/upstream/health_checker_impl_test.cc @@ -3840,8 +3840,7 @@ TEST_F(TcpHealthCheckerImplTest, PassiveFailureCrossThreadRemoveHostRace) { // Do a passive failure. This will not reset the active HC timers. Event::PostCb post_cb; - EXPECT_CALL(dispatcher_, post(testing::Matcher())) - .WillOnce(SaveArg<0>(&post_cb)); + EXPECT_CALL(dispatcher_, post(_)).WillOnce(SaveArg<0>(&post_cb)); cluster_->prioritySet().getMockHostSet(0)->hosts_[0]->healthChecker().setUnhealthy( HealthCheckHostMonitor::UnhealthyType::ImmediateHealthCheckFail); @@ -3871,8 +3870,7 @@ TEST_F(TcpHealthCheckerImplTest, PassiveFailureCrossThreadRemoveClusterRace) { // Do a passive failure. This will not reset the active HC timers. Event::PostCb post_cb; - EXPECT_CALL(dispatcher_, post(testing::Matcher())) - .WillOnce(SaveArg<0>(&post_cb)); + EXPECT_CALL(dispatcher_, post(_)).WillOnce(SaveArg<0>(&post_cb)); cluster_->prioritySet().getMockHostSet(0)->hosts_[0]->healthChecker().setUnhealthy( HealthCheckHostMonitor::UnhealthyType::ImmediateHealthCheckFail); diff --git a/test/common/upstream/original_dst_cluster_test.cc b/test/common/upstream/original_dst_cluster_test.cc index 653da5d5e04e..9ecd87ede145 100644 --- a/test/common/upstream/original_dst_cluster_test.cc +++ b/test/common/upstream/original_dst_cluster_test.cc @@ -205,7 +205,7 @@ TEST_F(OriginalDstClusterTest, NoContext) { { TestLoadBalancerContext lb_context(nullptr); OriginalDstCluster::LoadBalancer lb(cluster_); - EXPECT_CALL(dispatcher_, post(testing::Matcher())).Times(0); + EXPECT_CALL(dispatcher_, post(_)).Times(0); HostConstSharedPtr host = lb.chooseHost(&lb_context); EXPECT_EQ(host, nullptr); } @@ -219,7 +219,7 @@ TEST_F(OriginalDstClusterTest, NoContext) { // tests we do not have the thread local clusters, so we pass a reference to the HostSet of the // primary cluster. The implementation handles both cases the same. OriginalDstCluster::LoadBalancer lb(cluster_); - EXPECT_CALL(dispatcher_, post(testing::Matcher())).Times(0); + EXPECT_CALL(dispatcher_, post(_)).Times(0); HostConstSharedPtr host = lb.chooseHost(&lb_context); EXPECT_EQ(host, nullptr); } @@ -232,7 +232,7 @@ TEST_F(OriginalDstClusterTest, NoContext) { std::make_shared("unix://foo")); OriginalDstCluster::LoadBalancer lb(cluster_); - EXPECT_CALL(dispatcher_, post(testing::Matcher())).Times(0); + EXPECT_CALL(dispatcher_, post(_)).Times(0); HostConstSharedPtr host = lb.chooseHost(&lb_context); EXPECT_EQ(host, nullptr); } @@ -267,8 +267,7 @@ TEST_F(OriginalDstClusterTest, Membership) { std::make_shared("10.10.11.11")); Event::PostCb post_cb; - EXPECT_CALL(dispatcher_, post(testing::Matcher())) - .WillOnce(SaveArg<0>(&post_cb)); + EXPECT_CALL(dispatcher_, post(_)).WillOnce(SaveArg<0>(&post_cb)); // Mock the cluster manager by recreating the load balancer each time to get a fresh host map HostConstSharedPtr host = OriginalDstCluster::LoadBalancer(cluster_).chooseHost(&lb_context); post_cb(); @@ -317,8 +316,7 @@ TEST_F(OriginalDstClusterTest, Membership) { // New host gets created EXPECT_CALL(membership_updated_, ready()); - EXPECT_CALL(dispatcher_, post(testing::Matcher())) - .WillOnce(SaveArg<0>(&post_cb)); + EXPECT_CALL(dispatcher_, post(_)).WillOnce(SaveArg<0>(&post_cb)); // Mock the cluster manager by recreating the load balancer with the new host map HostConstSharedPtr host3 = OriginalDstCluster::LoadBalancer(cluster_).chooseHost(&lb_context); post_cb(); @@ -365,16 +363,14 @@ TEST_F(OriginalDstClusterTest, Membership2) { OriginalDstCluster::LoadBalancer lb(cluster_); EXPECT_CALL(membership_updated_, ready()); Event::PostCb post_cb; - EXPECT_CALL(dispatcher_, post(testing::Matcher())) - .WillOnce(SaveArg<0>(&post_cb)); + EXPECT_CALL(dispatcher_, post(_)).WillOnce(SaveArg<0>(&post_cb)); HostConstSharedPtr host1 = lb.chooseHost(&lb_context1); post_cb(); ASSERT_NE(host1, nullptr); EXPECT_EQ(*connection1.addressProvider().localAddress(), *host1->address()); EXPECT_CALL(membership_updated_, ready()); - EXPECT_CALL(dispatcher_, post(testing::Matcher())) - .WillOnce(SaveArg<0>(&post_cb)); + EXPECT_CALL(dispatcher_, post(_)).WillOnce(SaveArg<0>(&post_cb)); HostConstSharedPtr host2 = lb.chooseHost(&lb_context2); post_cb(); ASSERT_NE(host2, nullptr); @@ -450,8 +446,7 @@ TEST_F(OriginalDstClusterTest, Connection) { OriginalDstCluster::LoadBalancer lb(cluster_); Event::PostCb post_cb; - EXPECT_CALL(dispatcher_, post(testing::Matcher())) - .WillOnce(SaveArg<0>(&post_cb)); + EXPECT_CALL(dispatcher_, post(_)).WillOnce(SaveArg<0>(&post_cb)); HostConstSharedPtr host = lb.chooseHost(&lb_context); post_cb(); ASSERT_NE(host, nullptr); @@ -501,8 +496,7 @@ TEST_F(OriginalDstClusterTest, MultipleClusters) { OriginalDstCluster::LoadBalancer lb(cluster_); Event::PostCb post_cb; - EXPECT_CALL(dispatcher_, post(testing::Matcher())) - .WillOnce(SaveArg<0>(&post_cb)); + EXPECT_CALL(dispatcher_, post(_)).WillOnce(SaveArg<0>(&post_cb)); HostConstSharedPtr host = lb.chooseHost(&lb_context); post_cb(); ASSERT_NE(host, nullptr); @@ -545,8 +539,7 @@ TEST_F(OriginalDstClusterTest, UseHttpHeaderEnabled) { "127.0.0.1:5555"); EXPECT_CALL(membership_updated_, ready()); - EXPECT_CALL(dispatcher_, post(testing::Matcher())) - .WillOnce(SaveArg<0>(&post_cb)); + EXPECT_CALL(dispatcher_, post(_)).WillOnce(SaveArg<0>(&post_cb)); HostConstSharedPtr host1 = lb.chooseHost(&lb_context1); post_cb(); ASSERT_NE(host1, nullptr); @@ -560,8 +553,7 @@ TEST_F(OriginalDstClusterTest, UseHttpHeaderEnabled) { "127.0.0.1:5556"); EXPECT_CALL(membership_updated_, ready()); - EXPECT_CALL(dispatcher_, post(testing::Matcher())) - .WillOnce(SaveArg<0>(&post_cb)); + EXPECT_CALL(dispatcher_, post(_)).WillOnce(SaveArg<0>(&post_cb)); HostConstSharedPtr host2 = lb.chooseHost(&lb_context2); post_cb(); ASSERT_NE(host2, nullptr); @@ -571,7 +563,7 @@ TEST_F(OriginalDstClusterTest, UseHttpHeaderEnabled) { TestLoadBalancerContext lb_context3(nullptr, Http::Headers::get().EnvoyOriginalDstHost.get(), ""); EXPECT_CALL(membership_updated_, ready()).Times(0); - EXPECT_CALL(dispatcher_, post(testing::Matcher())).Times(0); + EXPECT_CALL(dispatcher_, post(_)).Times(0); HostConstSharedPtr host3 = lb.chooseHost(&lb_context3); EXPECT_EQ(host3, nullptr); EXPECT_EQ( @@ -582,7 +574,7 @@ TEST_F(OriginalDstClusterTest, UseHttpHeaderEnabled) { "a.b.c.d"); EXPECT_CALL(membership_updated_, ready()).Times(0); - EXPECT_CALL(dispatcher_, post(testing::Matcher())).Times(0); + EXPECT_CALL(dispatcher_, post(_)).Times(0); HostConstSharedPtr host4 = lb.chooseHost(&lb_context4); EXPECT_EQ(host4, nullptr); EXPECT_EQ( @@ -619,8 +611,7 @@ TEST_F(OriginalDstClusterTest, UseHttpHeaderDisabled) { "127.0.0.1:5555"); EXPECT_CALL(membership_updated_, ready()); - EXPECT_CALL(dispatcher_, post(testing::Matcher())) - .WillOnce(SaveArg<0>(&post_cb)); + EXPECT_CALL(dispatcher_, post(_)).WillOnce(SaveArg<0>(&post_cb)); HostConstSharedPtr host1 = lb.chooseHost(&lb_context1); post_cb(); ASSERT_NE(host1, nullptr); @@ -634,7 +625,7 @@ TEST_F(OriginalDstClusterTest, UseHttpHeaderDisabled) { "127.0.0.1:5555"); EXPECT_CALL(membership_updated_, ready()).Times(0); - EXPECT_CALL(dispatcher_, post(testing::Matcher())).Times(0); + EXPECT_CALL(dispatcher_, post(_)).Times(0); HostConstSharedPtr host2 = lb.chooseHost(&lb_context2); EXPECT_EQ(host2, nullptr); @@ -646,7 +637,7 @@ TEST_F(OriginalDstClusterTest, UseHttpHeaderDisabled) { "127.0.0.1:5555"); EXPECT_CALL(membership_updated_, ready()).Times(0); - EXPECT_CALL(dispatcher_, post(testing::Matcher())).Times(0); + EXPECT_CALL(dispatcher_, post(_)).Times(0); HostConstSharedPtr host3 = lb.chooseHost(&lb_context3); EXPECT_EQ(host3, nullptr); } diff --git a/test/common/upstream/outlier_detection_impl_test.cc b/test/common/upstream/outlier_detection_impl_test.cc index c4ca1f45cd41..1a985ddae542 100644 --- a/test/common/upstream/outlier_detection_impl_test.cc +++ b/test/common/upstream/outlier_detection_impl_test.cc @@ -1437,8 +1437,7 @@ TEST_F(OutlierDetectorImplTest, CrossThreadRemoveRace) { loadRq(hosts_[0], 4, 500); Event::PostCb post_cb; - EXPECT_CALL(dispatcher_, post(testing::Matcher())) - .WillOnce(SaveArg<0>(&post_cb)); + EXPECT_CALL(dispatcher_, post(_)).WillOnce(SaveArg<0>(&post_cb)); loadRq(hosts_[0], 1, 500); // Remove before the cross thread event comes in. @@ -1460,8 +1459,7 @@ TEST_F(OutlierDetectorImplTest, CrossThreadDestroyRace) { loadRq(hosts_[0], 4, 500); Event::PostCb post_cb; - EXPECT_CALL(dispatcher_, post(testing::Matcher())) - .WillOnce(SaveArg<0>(&post_cb)); + EXPECT_CALL(dispatcher_, post(_)).WillOnce(SaveArg<0>(&post_cb)); loadRq(hosts_[0], 1, 500); // Destroy before the cross thread event comes in. @@ -1484,8 +1482,7 @@ TEST_F(OutlierDetectorImplTest, CrossThreadFailRace) { loadRq(hosts_[0], 4, 500); Event::PostCb post_cb; - EXPECT_CALL(dispatcher_, post(testing::Matcher())) - .WillOnce(SaveArg<0>(&post_cb)); + EXPECT_CALL(dispatcher_, post(_)).WillOnce(SaveArg<0>(&post_cb)); loadRq(hosts_[0], 1, 500); time_system_.setMonotonicTime(std::chrono::milliseconds(0)); diff --git a/test/extensions/common/dynamic_forward_proxy/dns_cache_impl_test.cc b/test/extensions/common/dynamic_forward_proxy/dns_cache_impl_test.cc index 6d58d66b9be5..5eb41286c467 100644 --- a/test/extensions/common/dynamic_forward_proxy/dns_cache_impl_test.cc +++ b/test/extensions/common/dynamic_forward_proxy/dns_cache_impl_test.cc @@ -347,8 +347,7 @@ TEST_F(DnsCacheImplTest, InlineResolve) { MockLoadDnsCacheEntryCallbacks callbacks; Event::PostCb post_cb; - EXPECT_CALL(dispatcher_, post(testing::Matcher())) - .WillOnce(SaveArg<0>(&post_cb)); + EXPECT_CALL(dispatcher_, post(_)).WillOnce(SaveArg<0>(&post_cb)); auto result = dns_cache_->loadDnsCacheEntry("localhost", 80, callbacks); EXPECT_EQ(DnsCache::LoadDnsCacheEntryStatus::Loading, result.status_); EXPECT_NE(result.handle_, nullptr); diff --git a/test/extensions/transport_sockets/alts/tsi_handshaker_test.cc b/test/extensions/transport_sockets/alts/tsi_handshaker_test.cc index c8e66f0876ca..0b1afa2e0acc 100644 --- a/test/extensions/transport_sockets/alts/tsi_handshaker_test.cc +++ b/test/extensions/transport_sockets/alts/tsi_handshaker_test.cc @@ -152,8 +152,7 @@ TEST_F(TsiHandshakerTest, DeleteOnDone) { Buffer::OwnedImpl empty; std::function done; - EXPECT_CALL(dispatcher_, post(testing::Matcher())) - .WillOnce(SaveArg<0>(&done)); + EXPECT_CALL(dispatcher_, post(_)).WillOnce(SaveArg<0>(&done)); handshaker->next(empty); handshaker->deferredDelete(); diff --git a/test/mocks/event/mocks.cc b/test/mocks/event/mocks.cc index 9a131cf4cf8c..fae17cebddc4 100644 --- a/test/mocks/event/mocks.cc +++ b/test/mocks/event/mocks.cc @@ -27,7 +27,7 @@ MockDispatcher::MockDispatcher(const std::string& name) : name_(name) { ON_CALL(*this, createScaledTimer_(_, _)).WillByDefault(ReturnNew>()); ON_CALL(*this, createScaledTypedTimer_(_, _)) .WillByDefault(ReturnNew>()); - ON_CALL(*this, post(testing::Matcher())) + ON_CALL(*this, post(testing::Matcher())) .WillByDefault(Invoke([](const PostCb& cb) -> void { cb(); })); ON_CALL(*this, post(testing::Matcher())).WillByDefault(Invoke([](PostCb&& cb) -> void { cb(); diff --git a/test/server/connection_handler_test.cc b/test/server/connection_handler_test.cc index d273665916eb..dd55fea07a17 100644 --- a/test/server/connection_handler_test.cc +++ b/test/server/connection_handler_test.cc @@ -285,8 +285,7 @@ TEST_F(ConnectionHandlerTest, RemoveListenerDuringRebalance) { // Fake a balancer posting a connection to us. Event::PostCb post_cb; - EXPECT_CALL(dispatcher_, post(testing::Matcher())) - .WillOnce(SaveArg<0>(&post_cb)); + EXPECT_CALL(dispatcher_, post(_)).WillOnce(SaveArg<0>(&post_cb)); Network::MockConnectionSocket* connection = new NiceMock(); current_handler->incNumConnections(); #ifndef NDEBUG diff --git a/test/server/overload_manager_impl_test.cc b/test/server/overload_manager_impl_test.cc index 3d9411cd88f7..09d58bfa89e0 100644 --- a/test/server/overload_manager_impl_test.cc +++ b/test/server/overload_manager_impl_test.cc @@ -543,8 +543,7 @@ TEST_F(OverloadManagerImplTest, AdjustScaleFactor) { manager->start(); - EXPECT_CALL(mock_dispatcher, post(testing::Matcher())) - .WillOnce(InvokeArgument<0>()); + EXPECT_CALL(mock_dispatcher, post).WillOnce(InvokeArgument<0>()); // The scaled trigger has range [0.5, 1.0] so 0.6 should map to a scale value of 0.2, which means // a timer scale factor of 0.8 (1 - 0.2). EXPECT_CALL(*mock_scaled_timer_manager, From cf645764671e347af242270de75b8cd89b075625 Mon Sep 17 00:00:00 2001 From: Yuchen Dai Date: Wed, 10 Feb 2021 00:08:06 -0800 Subject: [PATCH 24/40] Revert "clean up movePost" This reverts commit 187179b0c801e25a2ed12da8bc767ff45bf05191. Signed-off-by: Yuchen Dai --- include/envoy/event/dispatcher.h | 8 ++++---- source/common/event/dispatcher_impl.cc | 4 ++-- source/common/event/dispatcher_impl.h | 4 ++-- source/common/upstream/upstream_impl.cc | 11 ++++++----- test/common/upstream/cluster_manager_impl_test.cc | 2 +- test/mocks/event/mocks.cc | 6 +----- test/mocks/event/mocks.h | 4 ++-- test/mocks/event/wrapped_dispatcher.h | 4 ++-- 8 files changed, 20 insertions(+), 23 deletions(-) diff --git a/include/envoy/event/dispatcher.h b/include/envoy/event/dispatcher.h index fed798270de6..f48521620f0c 100644 --- a/include/envoy/event/dispatcher.h +++ b/include/envoy/event/dispatcher.h @@ -258,13 +258,13 @@ class Dispatcher : public DispatcherBase { * Posts a functor to the dispatcher. This is safe cross thread. The functor runs in the context * of the dispatcher event loop which may be on a different thread than the caller. */ - virtual void post(const PostCb& callback) PURE; + virtual void post(PostCb callback) PURE; /** - * Similar to `post(const PostCb&)` but will destroy passed callback. This simulates posting move - * only function. + * Similar to `post()` but will destroy passed callback. This simulates posting move only + * function. */ - virtual void post(PostCb&& callback) PURE; + virtual void movePost(PostCb&& callback) PURE; /** * Runs the event loop. This will not return until exit() is called either from within a callback diff --git a/source/common/event/dispatcher_impl.cc b/source/common/event/dispatcher_impl.cc index 9d3398727164..35414ea6bc58 100644 --- a/source/common/event/dispatcher_impl.cc +++ b/source/common/event/dispatcher_impl.cc @@ -255,7 +255,7 @@ SignalEventPtr DispatcherImpl::listenForSignal(signal_t signal_num, SignalCb cb) return SignalEventPtr{new SignalEventImpl(*this, signal_num, cb)}; } -void DispatcherImpl::post(const PostCb& callback) { +void DispatcherImpl::post(std::function callback) { bool do_post; { Thread::LockGuard lock(post_lock_); @@ -268,7 +268,7 @@ void DispatcherImpl::post(const PostCb& callback) { } } -void DispatcherImpl::post(PostCb&& callback) { +void DispatcherImpl::movePost(std::function&& callback) { bool do_post; { Thread::LockGuard lock(post_lock_); diff --git a/source/common/event/dispatcher_impl.h b/source/common/event/dispatcher_impl.h index 76a902000dfb..3a16741eea95 100644 --- a/source/common/event/dispatcher_impl.h +++ b/source/common/event/dispatcher_impl.h @@ -85,8 +85,8 @@ class DispatcherImpl : Logger::Loggable, void deferredDelete(DeferredDeletablePtr&& to_delete) override; void exit() override; SignalEventPtr listenForSignal(signal_t signal_num, SignalCb cb) override; - void post(const PostCb& callback) override; - void post(PostCb&& callback) override; + void post(std::function callback) override; + void movePost(std::function&& callback) override; void run(RunType type) override; Buffer::WatermarkFactory& getWatermarkFactory() override { return *buffer_factory_; } void pushTrackedObject(const ScopeTrackedObject* object) override; diff --git a/source/common/upstream/upstream_impl.cc b/source/common/upstream/upstream_impl.cc index a5ab0c92f769..be1627177819 100644 --- a/source/common/upstream/upstream_impl.cc +++ b/source/common/upstream/upstream_impl.cc @@ -926,11 +926,12 @@ ClusterImplBase::ClusterImplBase( factory_context), [&dispatcher](const ClusterInfoImpl* self) { ENVOY_LOG(debug, "Schedule destroy cluster info {}", self->name()); - dispatcher.post([raii_cluster = std::shared_ptr(self)]() mutable { - ENVOY_LOG(debug, "Destroying cluster info {}. This thread should be master thread.", - raii_cluster->name()); - raii_cluster = nullptr; - }); + dispatcher.movePost( + [raii_cluster = std::shared_ptr(self)]() mutable { + ENVOY_LOG(debug, "Destroying cluster info {}. This thread should be master thread.", + raii_cluster->name()); + raii_cluster = nullptr; + }); // NOLINTNEXTLINE(clang-analyzer-cplusplus.NewDeleteLeaks) }); diff --git a/test/common/upstream/cluster_manager_impl_test.cc b/test/common/upstream/cluster_manager_impl_test.cc index 094bf420c664..653370c7db5c 100644 --- a/test/common/upstream/cluster_manager_impl_test.cc +++ b/test/common/upstream/cluster_manager_impl_test.cc @@ -874,7 +874,7 @@ TEST_F(ClusterManagerImplTest, HttpHealthChecker) { createClientConnection_( PointeesEq(Network::Utility::resolveUrl("tcp://127.0.0.1:11001")), _, _, _)) .WillOnce(Return(connection)); - EXPECT_CALL(factory_.dispatcher_, post(_)); + EXPECT_CALL(factory_.dispatcher_, movePost(_)); create(parseBootstrapFromV3Yaml(yaml)); factory_.tls_.shutdownThread(); factory_.dispatcher_.to_delete_.clear(); diff --git a/test/mocks/event/mocks.cc b/test/mocks/event/mocks.cc index fae17cebddc4..c3f2c766e202 100644 --- a/test/mocks/event/mocks.cc +++ b/test/mocks/event/mocks.cc @@ -27,11 +27,7 @@ MockDispatcher::MockDispatcher(const std::string& name) : name_(name) { ON_CALL(*this, createScaledTimer_(_, _)).WillByDefault(ReturnNew>()); ON_CALL(*this, createScaledTypedTimer_(_, _)) .WillByDefault(ReturnNew>()); - ON_CALL(*this, post(testing::Matcher())) - .WillByDefault(Invoke([](const PostCb& cb) -> void { cb(); })); - ON_CALL(*this, post(testing::Matcher())).WillByDefault(Invoke([](PostCb&& cb) -> void { - cb(); - })); + ON_CALL(*this, post(_)).WillByDefault(Invoke([](PostCb cb) -> void { cb(); })); ON_CALL(buffer_factory_, create_(_, _, _)) .WillByDefault(Invoke([](std::function below_low, std::function above_high, diff --git a/test/mocks/event/mocks.h b/test/mocks/event/mocks.h index 837fe54e064d..5c6ebafd040f 100644 --- a/test/mocks/event/mocks.h +++ b/test/mocks/event/mocks.h @@ -145,8 +145,8 @@ class MockDispatcher : public Dispatcher { MOCK_METHOD(void, deferredDelete_, (DeferredDeletable * to_delete)); MOCK_METHOD(void, exit, ()); MOCK_METHOD(SignalEvent*, listenForSignal_, (signal_t signal_num, SignalCb cb)); - MOCK_METHOD(void, post, (const PostCb& callback)); - MOCK_METHOD(void, post, (PostCb && callback)); + MOCK_METHOD(void, post, (std::function callback)); + MOCK_METHOD(void, movePost, (std::function && callback)); MOCK_METHOD(void, run, (RunType type)); MOCK_METHOD(void, pushTrackedObject, (const ScopeTrackedObject* object)); MOCK_METHOD(void, popTrackedObject, (const ScopeTrackedObject* expected_object)); diff --git a/test/mocks/event/wrapped_dispatcher.h b/test/mocks/event/wrapped_dispatcher.h index a53868329ddd..5bb600af035a 100644 --- a/test/mocks/event/wrapped_dispatcher.h +++ b/test/mocks/event/wrapped_dispatcher.h @@ -99,9 +99,9 @@ class WrappedDispatcher : public Dispatcher { return impl_.listenForSignal(signal_num, std::move(cb)); } - void post(const PostCb& callback) override { impl_.post(callback); } + void post(std::function callback) override { impl_.post(std::move(callback)); } - void post(PostCb&& callback) override { impl_.post(std::move(callback)); } + void movePost(std::function&& callback) override { impl_.movePost(std::move(callback)); } void run(RunType type) override { impl_.run(type); } From 40f25301b38026030bef16c42e68feb58cd0d145 Mon Sep 17 00:00:00 2001 From: Yuchen Dai Date: Wed, 10 Feb 2021 02:24:43 -0800 Subject: [PATCH 25/40] introduce DispatcherThreadDeletablePtr and replace movePost Signed-off-by: Yuchen Dai --- include/envoy/event/BUILD | 6 +++ include/envoy/event/dispatcher.h | 3 +- source/common/event/dispatcher_impl.cc | 37 +++++++++++++++---- source/common/event/dispatcher_impl.h | 6 ++- source/common/upstream/upstream_impl.cc | 9 +---- source/common/upstream/upstream_impl.h | 4 +- .../upstream/cluster_manager_impl_test.cc | 2 +- test/mocks/event/mocks.h | 2 +- test/mocks/event/wrapped_dispatcher.h | 4 +- 9 files changed, 51 insertions(+), 22 deletions(-) diff --git a/include/envoy/event/BUILD b/include/envoy/event/BUILD index 91d13f4d113c..c79ed56eb85e 100644 --- a/include/envoy/event/BUILD +++ b/include/envoy/event/BUILD @@ -13,11 +13,17 @@ envoy_cc_library( hdrs = ["deferred_deletable.h"], ) +envoy_cc_library( + name = "dispatcher_thread_deletable", + hdrs = ["dispatcher_thread_deletable.h"], +) + envoy_cc_library( name = "dispatcher_interface", hdrs = ["dispatcher.h"], deps = [ ":deferred_deletable", + ":dispatcher_thread_deletable", ":file_event_interface", ":scaled_timer", ":schedulable_cb_interface", diff --git a/include/envoy/event/dispatcher.h b/include/envoy/event/dispatcher.h index f48521620f0c..610dc63125cb 100644 --- a/include/envoy/event/dispatcher.h +++ b/include/envoy/event/dispatcher.h @@ -8,6 +8,7 @@ #include "envoy/common/scope_tracker.h" #include "envoy/common/time.h" +#include "envoy/event/dispatcher_thread_deletable.h" #include "envoy/event/file_event.h" #include "envoy/event/scaled_timer.h" #include "envoy/event/schedulable_cb.h" @@ -264,7 +265,7 @@ class Dispatcher : public DispatcherBase { * Similar to `post()` but will destroy passed callback. This simulates posting move only * function. */ - virtual void movePost(PostCb&& callback) PURE; + virtual void deleteInDispatcherThread(DispatcherThreadDeletablePtr deletable) PURE; /** * Runs the event loop. This will not return until exit() is called either from within a callback diff --git a/source/common/event/dispatcher_impl.cc b/source/common/event/dispatcher_impl.cc index 35414ea6bc58..7145d30fc71a 100644 --- a/source/common/event/dispatcher_impl.cc +++ b/source/common/event/dispatcher_impl.cc @@ -77,6 +77,10 @@ DispatcherImpl::DispatcherImpl(const std::string& name, Api::Api& api, DispatcherImpl::~DispatcherImpl() { ENVOY_LOG(debug, "destroying dispatcher {}", name_); FatalErrorHandler::removeFatalErrorHandler(*this); + // These deletable will be deleted anyway but it's violating the shutdown procedure. + ASSERT(deletables_in_dispatcher_thread_.empty(), + fmt::format("{} dispatcher contains {} dispatcher local deletables after shutdown.", + run_tid_.getId(), deletables_in_dispatcher_thread_.size())); } void DispatcherImpl::registerWatchdog(const Server::WatchDogSharedPtr& watchdog, @@ -259,7 +263,7 @@ void DispatcherImpl::post(std::function callback) { bool do_post; { Thread::LockGuard lock(post_lock_); - do_post = post_callbacks_.empty(); + do_post = post_callbacks_.empty() || deletables_in_dispatcher_thread_.empty(); post_callbacks_.push_back(callback); } @@ -268,14 +272,12 @@ void DispatcherImpl::post(std::function callback) { } } -void DispatcherImpl::movePost(std::function&& callback) { +void DispatcherImpl::deleteInDispatcherThread(DispatcherThreadDeletablePtr deletable) { bool do_post; { Thread::LockGuard lock(post_lock_); - do_post = post_callbacks_.empty(); - post_callbacks_.push_back(std::move(callback)); - // Poor man's move only function. - callback = nullptr; + do_post = post_callbacks_.empty() || deletables_in_dispatcher_thread_.empty(); + deletables_in_dispatcher_thread_.emplace_back(std::move(deletable)); } if (do_post) { @@ -298,10 +300,18 @@ MonotonicTime DispatcherImpl::approximateMonotonicTime() const { } void DispatcherImpl::shutdown() { - ENVOY_LOG(debug, "Clearing post callbacks in {}", __FUNCTION__); + ENVOY_LOG(debug, "Clearing dispatcher thread deletables in {}", __FUNCTION__); + std::list to_be_delete; { Thread::LockGuard lock(post_lock_); - post_callbacks_.clear(); + to_be_delete = std::move(deletables_in_dispatcher_thread_); + } + while (!to_be_delete.empty()) { + // Touch the watchdog before executing the callback to avoid spurious watchdog miss events when + // executing a long list of callbacks. + touchWatchdog(); + // FIFO. + to_be_delete.pop_front(); } } @@ -318,6 +328,7 @@ void DispatcherImpl::runPostCallbacks() { clearDeferredDeleteList(); std::list> callbacks; + std::list to_be_delete; { // Take ownership of the callbacks under the post_lock_. The lock must be released before // callbacks execute. Callbacks added after this transfer will re-arm post_cb_ and will execute @@ -326,6 +337,8 @@ void DispatcherImpl::runPostCallbacks() { callbacks = std::move(post_callbacks_); // post_callbacks_ should be empty after the move. ASSERT(post_callbacks_.empty()); + to_be_delete = std::move(deletables_in_dispatcher_thread_); + ASSERT(deletables_in_dispatcher_thread_.empty()); } // It is important that the execution and deletion of the callback happen while post_lock_ is not // held. Either the invocation or destructor of the callback can call post() on this dispatcher. @@ -339,6 +352,14 @@ void DispatcherImpl::runPostCallbacks() { // callback executes. callbacks.pop_front(); } + + while (!to_be_delete.empty()) { + // Touch the watchdog before executing the callback to avoid spurious watchdog miss events when + // executing a long list of callbacks. + touchWatchdog(); + // FIFO. + to_be_delete.pop_front(); + } } void DispatcherImpl::onFatalError(std::ostream& os) const { diff --git a/source/common/event/dispatcher_impl.h b/source/common/event/dispatcher_impl.h index 3a16741eea95..d0b37d0d1835 100644 --- a/source/common/event/dispatcher_impl.h +++ b/source/common/event/dispatcher_impl.h @@ -86,7 +86,7 @@ class DispatcherImpl : Logger::Loggable, void exit() override; SignalEventPtr listenForSignal(signal_t signal_num, SignalCb cb) override; void post(std::function callback) override; - void movePost(std::function&& callback) override; + void deleteInDispatcherThread(DispatcherThreadDeletablePtr deletable) override; void run(RunType type) override; Buffer::WatermarkFactory& getWatermarkFactory() override { return *buffer_factory_; } void pushTrackedObject(const ScopeTrackedObject* object) override; @@ -150,7 +150,9 @@ class DispatcherImpl : Logger::Loggable, SchedulableCallbackPtr deferred_delete_cb_; SchedulableCallbackPtr post_cb_; Thread::MutexBasicLockable post_lock_; - // `post_callback_` must be destroyed after `to_delete` because `to_delete` may post callbacks. + // `deletables_in_dispatcher_thread` must be destroyed last to allow other callbacks populate. + std::list + deletables_in_dispatcher_thread_ ABSL_GUARDED_BY(post_lock_); std::list> post_callbacks_ ABSL_GUARDED_BY(post_lock_); std::vector to_delete_1_; std::vector to_delete_2_; diff --git a/source/common/upstream/upstream_impl.cc b/source/common/upstream/upstream_impl.cc index be1627177819..a34b69531f25 100644 --- a/source/common/upstream/upstream_impl.cc +++ b/source/common/upstream/upstream_impl.cc @@ -926,13 +926,8 @@ ClusterImplBase::ClusterImplBase( factory_context), [&dispatcher](const ClusterInfoImpl* self) { ENVOY_LOG(debug, "Schedule destroy cluster info {}", self->name()); - dispatcher.movePost( - [raii_cluster = std::shared_ptr(self)]() mutable { - ENVOY_LOG(debug, "Destroying cluster info {}. This thread should be master thread.", - raii_cluster->name()); - raii_cluster = nullptr; - }); - // NOLINTNEXTLINE(clang-analyzer-cplusplus.NewDeleteLeaks) + dispatcher.deleteInDispatcherThread( + std::unique_ptr(const_cast(self))); }); if ((info_->features() & ClusterInfoImpl::Features::USE_ALPN) && diff --git a/source/common/upstream/upstream_impl.h b/source/common/upstream/upstream_impl.h index 6addfd626b99..2bc595cd5130 100644 --- a/source/common/upstream/upstream_impl.h +++ b/source/common/upstream/upstream_impl.h @@ -509,7 +509,9 @@ class PrioritySetImpl : public PrioritySet { /** * Implementation of ClusterInfo that reads from JSON. */ -class ClusterInfoImpl : public ClusterInfo, protected Logger::Loggable { +class ClusterInfoImpl : public ClusterInfo, + public Event::DispatcherThreadDeletable, + protected Logger::Loggable { public: using HttpProtocolOptionsConfigImpl = Envoy::Extensions::Upstreams::Http::ProtocolOptionsConfigImpl; diff --git a/test/common/upstream/cluster_manager_impl_test.cc b/test/common/upstream/cluster_manager_impl_test.cc index 653370c7db5c..6428a62da4a9 100644 --- a/test/common/upstream/cluster_manager_impl_test.cc +++ b/test/common/upstream/cluster_manager_impl_test.cc @@ -874,7 +874,7 @@ TEST_F(ClusterManagerImplTest, HttpHealthChecker) { createClientConnection_( PointeesEq(Network::Utility::resolveUrl("tcp://127.0.0.1:11001")), _, _, _)) .WillOnce(Return(connection)); - EXPECT_CALL(factory_.dispatcher_, movePost(_)); + EXPECT_CALL(factory_.dispatcher_, deleteInDispatcherThread(_)); create(parseBootstrapFromV3Yaml(yaml)); factory_.tls_.shutdownThread(); factory_.dispatcher_.to_delete_.clear(); diff --git a/test/mocks/event/mocks.h b/test/mocks/event/mocks.h index 5c6ebafd040f..9eda2ef636a9 100644 --- a/test/mocks/event/mocks.h +++ b/test/mocks/event/mocks.h @@ -146,7 +146,7 @@ class MockDispatcher : public Dispatcher { MOCK_METHOD(void, exit, ()); MOCK_METHOD(SignalEvent*, listenForSignal_, (signal_t signal_num, SignalCb cb)); MOCK_METHOD(void, post, (std::function callback)); - MOCK_METHOD(void, movePost, (std::function && callback)); + MOCK_METHOD(void, deleteInDispatcherThread, (DispatcherThreadDeletablePtr deletable)); MOCK_METHOD(void, run, (RunType type)); MOCK_METHOD(void, pushTrackedObject, (const ScopeTrackedObject* object)); MOCK_METHOD(void, popTrackedObject, (const ScopeTrackedObject* expected_object)); diff --git a/test/mocks/event/wrapped_dispatcher.h b/test/mocks/event/wrapped_dispatcher.h index 5bb600af035a..18f99ac58c5f 100644 --- a/test/mocks/event/wrapped_dispatcher.h +++ b/test/mocks/event/wrapped_dispatcher.h @@ -101,7 +101,9 @@ class WrappedDispatcher : public Dispatcher { void post(std::function callback) override { impl_.post(std::move(callback)); } - void movePost(std::function&& callback) override { impl_.movePost(std::move(callback)); } + void deleteInDispatcherThread(DispatcherThreadDeletablePtr deletable) override { + impl_.deleteInDispatcherThread(std::move(deletable)); + } void run(RunType type) override { impl_.run(type); } From a5c2c3c5d46fd1e774f27249a4596fbdd03990ef Mon Sep 17 00:00:00 2001 From: Yuchen Dai Date: Wed, 10 Feb 2021 02:39:17 -0800 Subject: [PATCH 26/40] add mising file Signed-off-by: Yuchen Dai --- .../envoy/event/dispatcher_thread_deletable.h | 16 ++++++++++++++++ 1 file changed, 16 insertions(+) create mode 100644 include/envoy/event/dispatcher_thread_deletable.h diff --git a/include/envoy/event/dispatcher_thread_deletable.h b/include/envoy/event/dispatcher_thread_deletable.h new file mode 100644 index 000000000000..667059c0fd5e --- /dev/null +++ b/include/envoy/event/dispatcher_thread_deletable.h @@ -0,0 +1,16 @@ +#pragma once + +#include + +namespace Envoy { +namespace Event { + +class DispatcherThreadDeletable { +public: + virtual ~DispatcherThreadDeletable() = default; +}; + +using DispatcherThreadDeletablePtr = std::unique_ptr; + +} // namespace Event +} // namespace Envoy From 30c9751086f861e97b00c3d3b19124cd8b67c07b Mon Sep 17 00:00:00 2001 From: Yuchen Dai Date: Wed, 10 Feb 2021 10:38:51 -0800 Subject: [PATCH 27/40] remove assert dispatcher-thread-deletable, add comment, fix sds test asan failure Signed-off-by: Yuchen Dai --- include/envoy/event/dispatcher_thread_deletable.h | 5 +++++ source/common/event/dispatcher_impl.cc | 5 +---- test/integration/sds_dynamic_integration_test.cc | 12 +++++++++--- 3 files changed, 15 insertions(+), 7 deletions(-) diff --git a/include/envoy/event/dispatcher_thread_deletable.h b/include/envoy/event/dispatcher_thread_deletable.h index 667059c0fd5e..1416456603d0 100644 --- a/include/envoy/event/dispatcher_thread_deletable.h +++ b/include/envoy/event/dispatcher_thread_deletable.h @@ -5,6 +5,11 @@ namespace Envoy { namespace Event { +/** + * If an object derives from this class, it can be passed to the destination dispatcher who + * guarantees to delete it in that dispatcher thread. The common use case is to ensure config + * related objects are deleted in the main thread. + */ class DispatcherThreadDeletable { public: virtual ~DispatcherThreadDeletable() = default; diff --git a/source/common/event/dispatcher_impl.cc b/source/common/event/dispatcher_impl.cc index 7145d30fc71a..21cb28b7740a 100644 --- a/source/common/event/dispatcher_impl.cc +++ b/source/common/event/dispatcher_impl.cc @@ -77,10 +77,7 @@ DispatcherImpl::DispatcherImpl(const std::string& name, Api::Api& api, DispatcherImpl::~DispatcherImpl() { ENVOY_LOG(debug, "destroying dispatcher {}", name_); FatalErrorHandler::removeFatalErrorHandler(*this); - // These deletable will be deleted anyway but it's violating the shutdown procedure. - ASSERT(deletables_in_dispatcher_thread_.empty(), - fmt::format("{} dispatcher contains {} dispatcher local deletables after shutdown.", - run_tid_.getId(), deletables_in_dispatcher_thread_.size())); + // TODO(lambdai): Explore the cases that `deletables_in_dispatcher_thread_` is not empty(). } void DispatcherImpl::registerWatchdog(const Server::WatchDogSharedPtr& watchdog, diff --git a/test/integration/sds_dynamic_integration_test.cc b/test/integration/sds_dynamic_integration_test.cc index 20fa9fc03194..c1dcb3501a8b 100644 --- a/test/integration/sds_dynamic_integration_test.cc +++ b/test/integration/sds_dynamic_integration_test.cc @@ -673,7 +673,7 @@ TEST_P(SdsDynamicUpstreamIntegrationTest, WrongSecretFirst) { EXPECT_EQ(1, test_server_->counter("sds.client_cert.update_rejected")->value()); } -// Test CDS with SDS. +// Test CDS with SDS. A cluster provided by CDS raises new SDS request for upstream cert. class SdsCdsIntegrationTest : public SdsDynamicIntegrationBaseTest { public: void initialize() override { @@ -697,7 +697,7 @@ class SdsCdsIntegrationTest : public SdsDynamicIntegrationBaseTest { cds_cluster->MergeFrom(bootstrap.static_resources().clusters()[0]); cds_cluster->set_name("cds_cluster"); ConfigHelper::setHttp2(*cds_cluster); - // Then add sds cluster first. + // Then add sds cluster. auto* sds_cluster = bootstrap.mutable_static_resources()->add_clusters(); sds_cluster->MergeFrom(bootstrap.static_resources().clusters()[0]); sds_cluster->set_name("sds_cluster"); @@ -718,10 +718,16 @@ class SdsCdsIntegrationTest : public SdsDynamicIntegrationBaseTest { }); HttpIntegrationTest::initialize(); - // registerTestServerPorts({"http"}); } void TearDown() override { + { + AssertionResult result = sds_connection_->close(); + RELEASE_ASSERT(result, result.message()); + result = sds_connection_->waitForDisconnect(); + RELEASE_ASSERT(result, result.message()); + sds_connection_.reset(); + } cleanUpXdsConnection(); cleanupUpstreamAndDownstream(); codec_client_.reset(); From 13379da0c9dfeb10b605f73d60790bfd666740dc Mon Sep 17 00:00:00 2001 From: Yuchen Dai Date: Wed, 10 Feb 2021 19:38:38 -0800 Subject: [PATCH 28/40] use independent thread_local_delete_cb_ in dispatcher Signed-off-by: Yuchen Dai --- include/envoy/event/dispatcher.h | 6 ++-- source/common/event/dispatcher_impl.cc | 49 ++++++++++++-------------- source/common/event/dispatcher_impl.h | 14 ++++++-- test/server/server_test.cc | 2 +- 4 files changed, 37 insertions(+), 34 deletions(-) diff --git a/include/envoy/event/dispatcher.h b/include/envoy/event/dispatcher.h index 610dc63125cb..ea12fff4d7aa 100644 --- a/include/envoy/event/dispatcher.h +++ b/include/envoy/event/dispatcher.h @@ -262,8 +262,8 @@ class Dispatcher : public DispatcherBase { virtual void post(PostCb callback) PURE; /** - * Similar to `post()` but will destroy passed callback. This simulates posting move only - * function. + * Post the deletable to this dispatcher. The deletable objects are guaranteed to be destroyed on + * the dispatcher's thread before dispatcher destroy. This is safe cross thread. */ virtual void deleteInDispatcherThread(DispatcherThreadDeletablePtr deletable) PURE; @@ -296,7 +296,7 @@ class Dispatcher : public DispatcherBase { virtual void updateApproximateMonotonicTime() PURE; /** - * Shutdown the dispatcher by removing posted callbacks. + * Shutdown the dispatcher by clear dispatcher thread deletable. */ virtual void shutdown() PURE; }; diff --git a/source/common/event/dispatcher_impl.cc b/source/common/event/dispatcher_impl.cc index 21cb28b7740a..759dfe09f141 100644 --- a/source/common/event/dispatcher_impl.cc +++ b/source/common/event/dispatcher_impl.cc @@ -63,6 +63,8 @@ DispatcherImpl::DispatcherImpl(const std::string& name, Api::Api& api, ? watermark_factory : std::make_shared()), scheduler_(time_system.createScheduler(base_scheduler_, base_scheduler_)), + thread_local_delete_cb_( + base_scheduler_.createSchedulableCallback([this]() -> void { runThreadLocalDelete(); })), deferred_delete_cb_(base_scheduler_.createSchedulableCallback( [this]() -> void { clearDeferredDeleteList(); })), post_cb_(base_scheduler_.createSchedulableCallback([this]() -> void { runPostCallbacks(); })), @@ -260,7 +262,7 @@ void DispatcherImpl::post(std::function callback) { bool do_post; { Thread::LockGuard lock(post_lock_); - do_post = post_callbacks_.empty() || deletables_in_dispatcher_thread_.empty(); + do_post = post_callbacks_.empty(); post_callbacks_.push_back(callback); } @@ -270,15 +272,15 @@ void DispatcherImpl::post(std::function callback) { } void DispatcherImpl::deleteInDispatcherThread(DispatcherThreadDeletablePtr deletable) { - bool do_post; + bool do_delete; { - Thread::LockGuard lock(post_lock_); - do_post = post_callbacks_.empty() || deletables_in_dispatcher_thread_.empty(); + Thread::LockGuard lock(thread_local_deletable_lock_); + do_delete = deletables_in_dispatcher_thread_.empty(); deletables_in_dispatcher_thread_.emplace_back(std::move(deletable)); } - if (do_post) { - post_cb_->scheduleCallbackCurrentIteration(); + if (do_delete) { + thread_local_delete_cb_->scheduleCallbackCurrentIteration(); } } @@ -298,26 +300,30 @@ MonotonicTime DispatcherImpl::approximateMonotonicTime() const { void DispatcherImpl::shutdown() { ENVOY_LOG(debug, "Clearing dispatcher thread deletables in {}", __FUNCTION__); + runThreadLocalDelete(); +} + +void DispatcherImpl::updateApproximateMonotonicTime() { updateApproximateMonotonicTimeInternal(); } + +void DispatcherImpl::updateApproximateMonotonicTimeInternal() { + approximate_monotonic_time_ = api_.timeSource().monotonicTime(); +} + +void DispatcherImpl::runThreadLocalDelete() { std::list to_be_delete; { - Thread::LockGuard lock(post_lock_); + Thread::LockGuard lock(thread_local_deletable_lock_); to_be_delete = std::move(deletables_in_dispatcher_thread_); + ASSERT(deletables_in_dispatcher_thread_.empty()); } while (!to_be_delete.empty()) { // Touch the watchdog before executing the callback to avoid spurious watchdog miss events when - // executing a long list of callbacks. + // executing complicated destruction. touchWatchdog(); - // FIFO. + // Delete in FIFO order. to_be_delete.pop_front(); } } - -void DispatcherImpl::updateApproximateMonotonicTime() { updateApproximateMonotonicTimeInternal(); } - -void DispatcherImpl::updateApproximateMonotonicTimeInternal() { - approximate_monotonic_time_ = api_.timeSource().monotonicTime(); -} - void DispatcherImpl::runPostCallbacks() { // Clear the deferred delete list before running post callbacks to reduce non-determinism in // callback processing, and more easily detect if a scheduled post callback refers to one of the @@ -325,7 +331,6 @@ void DispatcherImpl::runPostCallbacks() { clearDeferredDeleteList(); std::list> callbacks; - std::list to_be_delete; { // Take ownership of the callbacks under the post_lock_. The lock must be released before // callbacks execute. Callbacks added after this transfer will re-arm post_cb_ and will execute @@ -334,8 +339,6 @@ void DispatcherImpl::runPostCallbacks() { callbacks = std::move(post_callbacks_); // post_callbacks_ should be empty after the move. ASSERT(post_callbacks_.empty()); - to_be_delete = std::move(deletables_in_dispatcher_thread_); - ASSERT(deletables_in_dispatcher_thread_.empty()); } // It is important that the execution and deletion of the callback happen while post_lock_ is not // held. Either the invocation or destructor of the callback can call post() on this dispatcher. @@ -349,14 +352,6 @@ void DispatcherImpl::runPostCallbacks() { // callback executes. callbacks.pop_front(); } - - while (!to_be_delete.empty()) { - // Touch the watchdog before executing the callback to avoid spurious watchdog miss events when - // executing a long list of callbacks. - touchWatchdog(); - // FIFO. - to_be_delete.pop_front(); - } } void DispatcherImpl::onFatalError(std::ostream& os) const { diff --git a/source/common/event/dispatcher_impl.h b/source/common/event/dispatcher_impl.h index d0b37d0d1835..a3737253cf23 100644 --- a/source/common/event/dispatcher_impl.h +++ b/source/common/event/dispatcher_impl.h @@ -129,6 +129,8 @@ class DispatcherImpl : Logger::Loggable, TimerPtr createTimerInternal(TimerCb cb); void updateApproximateMonotonicTimeInternal(); void runPostCallbacks(); + void runThreadLocalDelete(); + // Helper used to touch the watchdog after most schedulable, fd, and timer callbacks. void touchWatchdog(); @@ -147,13 +149,19 @@ class DispatcherImpl : Logger::Loggable, Buffer::WatermarkFactorySharedPtr buffer_factory_; LibeventScheduler base_scheduler_; SchedulerPtr scheduler_; + + SchedulableCallbackPtr thread_local_delete_cb_; + Thread::MutexBasicLockable thread_local_deletable_lock_; + // `deletables_in_dispatcher_thread` must be destroyed last to allow other callbacks populate. + std::list + deletables_in_dispatcher_thread_ ABSL_GUARDED_BY(thread_local_deletable_lock_); + SchedulableCallbackPtr deferred_delete_cb_; + SchedulableCallbackPtr post_cb_; Thread::MutexBasicLockable post_lock_; - // `deletables_in_dispatcher_thread` must be destroyed last to allow other callbacks populate. - std::list - deletables_in_dispatcher_thread_ ABSL_GUARDED_BY(post_lock_); std::list> post_callbacks_ ABSL_GUARDED_BY(post_lock_); + std::vector to_delete_1_; std::vector to_delete_2_; std::vector* current_to_delete_; diff --git a/test/server/server_test.cc b/test/server/server_test.cc index 90234fda8b97..8695c0d0255d 100644 --- a/test/server/server_test.cc +++ b/test/server/server_test.cc @@ -268,8 +268,8 @@ class ServerInstanceImplTestBase { testing::NiceMock options_; DefaultListenerHooks hooks_; testing::NiceMock restart_; - Stats::TestIsolatedStoreImpl stats_store_; ThreadLocal::InstanceImplPtr thread_local_; + Stats::TestIsolatedStoreImpl stats_store_; Thread::MutexBasicLockable fakelock_; TestComponentFactory component_factory_; Event::GlobalTimeSystem time_system_; From cc0f944b2c8d6a485d617adacc6ffbb47d573148 Mon Sep 17 00:00:00 2001 From: Yuchen Dai Date: Wed, 10 Feb 2021 22:35:39 -0800 Subject: [PATCH 29/40] revert server.h Signed-off-by: Yuchen Dai --- source/server/config_validation/server.h | 14 +++++++------- 1 file changed, 7 insertions(+), 7 deletions(-) diff --git a/source/server/config_validation/server.h b/source/server/config_validation/server.h index 5e4cf6c9fbba..e735b1ecd183 100644 --- a/source/server/config_validation/server.h +++ b/source/server/config_validation/server.h @@ -174,12 +174,7 @@ class ValidationInstance final : Logger::Loggable, void initialize(const Options& options, const Network::Address::InstanceConstSharedPtr& local_address, ComponentFactory& component_factory); - const Options& options_; - ProtobufMessage::ProdValidationContextImpl validation_context_; - Stats::IsolatedStoreImpl& stats_store_; - Api::ApiPtr api_; - Event::DispatcherPtr dispatcher_; - ThreadLocal::InstanceImpl thread_local_; + // init_manager_ must come before any member that participates in initialization, and destructed // only after referencing members are gone, since initialization continuation can potentially // occur at any point during member lifetime. @@ -191,7 +186,12 @@ class ValidationInstance final : Logger::Loggable, // - There may be active clusters referencing it in config_.cluster_manager_. // - There may be active connections referencing it. std::unique_ptr secret_manager_; - + const Options& options_; + ProtobufMessage::ProdValidationContextImpl validation_context_; + Stats::IsolatedStoreImpl& stats_store_; + ThreadLocal::InstanceImpl thread_local_; + Api::ApiPtr api_; + Event::DispatcherPtr dispatcher_; Server::ValidationAdmin admin_; Singleton::ManagerPtr singleton_manager_; std::unique_ptr runtime_singleton_; From 8680fa7e4b6cb45f905481c6a39fcb28fdf20059 Mon Sep 17 00:00:00 2001 From: Yuchen Dai Date: Tue, 16 Feb 2021 03:30:45 -0800 Subject: [PATCH 30/40] check shutdown_called_ Signed-off-by: Yuchen Dai --- include/envoy/event/dispatcher.h | 4 +- source/common/event/dispatcher_impl.cc | 60 ++++++++++++++++++++--- source/common/event/dispatcher_impl.h | 1 + source/server/worker_impl.cc | 1 + test/common/event/dispatcher_impl_test.cc | 37 +++++++++++++- 5 files changed, 93 insertions(+), 10 deletions(-) diff --git a/include/envoy/event/dispatcher.h b/include/envoy/event/dispatcher.h index ea12fff4d7aa..90f405d8d00e 100644 --- a/include/envoy/event/dispatcher.h +++ b/include/envoy/event/dispatcher.h @@ -263,7 +263,7 @@ class Dispatcher : public DispatcherBase { /** * Post the deletable to this dispatcher. The deletable objects are guaranteed to be destroyed on - * the dispatcher's thread before dispatcher destroy. This is safe cross thread. + * the dispatcher's thread before the dispatcher is destroyed. This is safe cross thread. */ virtual void deleteInDispatcherThread(DispatcherThreadDeletablePtr deletable) PURE; @@ -296,7 +296,7 @@ class Dispatcher : public DispatcherBase { virtual void updateApproximateMonotonicTime() PURE; /** - * Shutdown the dispatcher by clear dispatcher thread deletable. + * Shutdown the dispatcher by clearing the dispatcher's thread deletable objects. */ virtual void shutdown() PURE; }; diff --git a/source/common/event/dispatcher_impl.cc b/source/common/event/dispatcher_impl.cc index 759dfe09f141..e7c05f0784a3 100644 --- a/source/common/event/dispatcher_impl.cc +++ b/source/common/event/dispatcher_impl.cc @@ -77,9 +77,12 @@ DispatcherImpl::DispatcherImpl(const std::string& name, Api::Api& api, } DispatcherImpl::~DispatcherImpl() { + // TODO(lambdai): call shutdown() in tests and assert shutdown_called_. ENVOY_LOG(debug, "destroying dispatcher {}", name_); FatalErrorHandler::removeFatalErrorHandler(*this); - // TODO(lambdai): Explore the cases that `deletables_in_dispatcher_thread_` is not empty(). + ASSERT(deletables_in_dispatcher_thread_.empty(), + fmt::format("{} dispatcher contains {} dispatcher local deletables after shutdown.", + run_tid_.getId(), deletables_in_dispatcher_thread_.size())); } void DispatcherImpl::registerWatchdog(const Server::WatchDogSharedPtr& watchdog, @@ -272,14 +275,15 @@ void DispatcherImpl::post(std::function callback) { } void DispatcherImpl::deleteInDispatcherThread(DispatcherThreadDeletablePtr deletable) { - bool do_delete; + bool need_schedule; { Thread::LockGuard lock(thread_local_deletable_lock_); - do_delete = deletables_in_dispatcher_thread_.empty(); + need_schedule = deletables_in_dispatcher_thread_.empty(); deletables_in_dispatcher_thread_.emplace_back(std::move(deletable)); + ASSERT(!shutdown_called_, "inserted after shutdown"); } - if (do_delete) { + if (need_schedule) { thread_local_delete_cb_->scheduleCallbackCurrentIteration(); } } @@ -299,8 +303,50 @@ MonotonicTime DispatcherImpl::approximateMonotonicTime() const { } void DispatcherImpl::shutdown() { - ENVOY_LOG(debug, "Clearing dispatcher thread deletables in {}", __FUNCTION__); - runThreadLocalDelete(); + ASSERT(isThreadSafe()); + + bool delete_again = true; + int round = 0; + while (delete_again) { + delete_again = false; + round++; + auto deferred_deletables_size = current_to_delete_->size(); + delete_again |= deferred_deletables_size > 0; + clearDeferredDeleteList(); + + std::list> callbacks; + { + Thread::LockGuard lock(post_lock_); + callbacks = std::move(post_callbacks_); + } + + auto post_callbacks_size = callbacks.size(); + while (!callbacks.empty()) { + // Don't invoke callbacks when the dispatcher is shutting down. + callbacks.pop_front(); + } + delete_again |= post_callbacks_size > 0; + + std::list local_deletables; + { + Thread::LockGuard lock(thread_local_deletable_lock_); + local_deletables = std::move(deletables_in_dispatcher_thread_); + } + auto thread_local_deletables_size = local_deletables.size(); + while (!local_deletables.empty()) { + local_deletables.pop_front(); + } + + delete_again |= thread_local_deletables_size > 0; + ENVOY_LOG(debug, + "Round {}: destroyed {} deferred deletables, {} post callbacks and {} thread local " + "deletables in {}", + round, deferred_deletables_size, post_callbacks_size, thread_local_deletables_size, + __FUNCTION__); + } + + ASSERT(!shutdown_called_); + shutdown_called_ = true; } void DispatcherImpl::updateApproximateMonotonicTime() { updateApproximateMonotonicTimeInternal(); } @@ -317,7 +363,7 @@ void DispatcherImpl::runThreadLocalDelete() { ASSERT(deletables_in_dispatcher_thread_.empty()); } while (!to_be_delete.empty()) { - // Touch the watchdog before executing the callback to avoid spurious watchdog miss events when + // Touch the watchdog before deleting the objects to avoid spurious watchdog miss events when // executing complicated destruction. touchWatchdog(); // Delete in FIFO order. diff --git a/source/common/event/dispatcher_impl.h b/source/common/event/dispatcher_impl.h index a3737253cf23..5c9568ae076e 100644 --- a/source/common/event/dispatcher_impl.h +++ b/source/common/event/dispatcher_impl.h @@ -155,6 +155,7 @@ class DispatcherImpl : Logger::Loggable, // `deletables_in_dispatcher_thread` must be destroyed last to allow other callbacks populate. std::list deletables_in_dispatcher_thread_ ABSL_GUARDED_BY(thread_local_deletable_lock_); + bool shutdown_called_{false}; SchedulableCallbackPtr deferred_delete_cb_; diff --git a/source/server/worker_impl.cc b/source/server/worker_impl.cc index 95c24175321c..b7d54e28d0a2 100644 --- a/source/server/worker_impl.cc +++ b/source/server/worker_impl.cc @@ -109,6 +109,7 @@ void WorkerImpl::stop() { // is happening, so we might not yet have a thread. if (thread_) { dispatcher_->exit(); + dispatcher_->shutdown(); thread_->join(); } } diff --git a/test/common/event/dispatcher_impl_test.cc b/test/common/event/dispatcher_impl_test.cc index 198a0ef4fd8b..a0b5785d134e 100644 --- a/test/common/event/dispatcher_impl_test.cc +++ b/test/common/event/dispatcher_impl_test.cc @@ -33,6 +33,30 @@ using testing::Return; namespace Envoy { namespace Event { namespace { +class Destroyable : public DeferredDeletable, public DispatcherThreadDeletable { +public: + Destroyable(Dispatcher& dispatcher, int life, ReadyWatcher& watcher) + : dispatcher_(dispatcher), life_(life), watcher_(watcher) {} + ~Destroyable() { + + if (life_ != 0) { + if ((life_ % 3) == 0) { + dispatcher_.post([obj = std::make_shared( + dispatcher_, life_ - 1, watcher_)]() mutable { obj.reset(); }); + } else if ((life_ % 3) == 1) { + dispatcher_.deleteInDispatcherThread( + std::make_unique(dispatcher_, life_ - 1, watcher_)); + } else { + dispatcher_.deferredDelete(std::make_unique(dispatcher_, life_ - 1, watcher_)); + } + } else { + watcher_.ready(); + } + } + Dispatcher& dispatcher_; + uint32_t life_; + ReadyWatcher& watcher_; +}; class RunOnDelete { public: @@ -358,7 +382,6 @@ class DispatcherImplTest : public testing::Test { cv_.wait(mu_); } } - NiceMock scope_; // Used in InitializeStats, must outlive dispatcher_->exit(). Api::ApiPtr api_; Thread::ThreadPtr dispatcher_thread_; @@ -442,6 +465,18 @@ TEST_F(DispatcherImplTest, PostExecuteAndDestructOrder) { } } +TEST(DispatcherShutdownTest, ShutdownCleanupAllDestroyableObjects) { + auto api = Api::createApiForTest(); + auto dispatcher = api->allocateDispatcher("test_thread"); + ReadyWatcher shutdown_watcher; + dispatcher->deferredDelete(std::make_unique(*dispatcher, 10, shutdown_watcher)); + + EXPECT_CALL(shutdown_watcher, ready()); + dispatcher->shutdown(); + // Need to verify before dispatcher is destroyed. + testing::Mock::VerifyAndClearExpectations(&shutdown_watcher); +} + // Ensure that there is no deadlock related to calling a posted callback, or // destructing a closure when finished calling it. TEST_F(DispatcherImplTest, RunPostCallbacksLocking) { From 5af6d37c0c4424bd69b60851e802d9202b75d93f Mon Sep 17 00:00:00 2001 From: Yuchen Dai Date: Tue, 16 Feb 2021 13:22:05 -0800 Subject: [PATCH 31/40] call worker dispatcher shutdown at worker thread Signed-off-by: Yuchen Dai --- source/server/worker_impl.cc | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/source/server/worker_impl.cc b/source/server/worker_impl.cc index b7d54e28d0a2..aaece18609ce 100644 --- a/source/server/worker_impl.cc +++ b/source/server/worker_impl.cc @@ -109,7 +109,6 @@ void WorkerImpl::stop() { // is happening, so we might not yet have a thread. if (thread_) { dispatcher_->exit(); - dispatcher_->shutdown(); thread_->join(); } } @@ -136,6 +135,7 @@ void WorkerImpl::threadRoutine(GuardDog& guard_dog) { dispatcher_->run(Event::Dispatcher::RunType::Block); ENVOY_LOG(debug, "worker exited dispatch loop"); guard_dog.stopWatching(watch_dog_); + dispatcher_->shutdown(); // We must close all active connections before we actually exit the thread. This prevents any // destructors from running on the main thread which might reference thread locals. Destroying From 93e0b0f15e2175632d5b8f5a0f918d8496e54974 Mon Sep 17 00:00:00 2001 From: Yuchen Dai Date: Wed, 17 Feb 2021 01:33:17 -0800 Subject: [PATCH 32/40] fail to fix dispatcher shutdown Signed-off-by: Yuchen Dai --- source/common/config/grpc_mux_impl.h | 7 ++++--- source/common/config/grpc_stream.h | 6 +++++- source/common/grpc/async_client_impl.cc | 3 +++ source/common/grpc/typed_async_client.h | 5 ++++- source/server/server.cc | 3 ++- source/server/server.h | 7 +++++++ 6 files changed, 25 insertions(+), 6 deletions(-) diff --git a/source/common/config/grpc_mux_impl.h b/source/common/config/grpc_mux_impl.h index 4b46dbb67317..e89c1d4b0886 100644 --- a/source/common/config/grpc_mux_impl.h +++ b/source/common/config/grpc_mux_impl.h @@ -161,13 +161,14 @@ class GrpcMuxImpl : public GrpcMux, absl::node_hash_map> api_state_; - // Envoy's dependency ordering. - std::list subscriptions_; - // A queue to store requests while rate limited. Note that when requests cannot be sent due to the // gRPC stream being down, this queue does not store them; rather, they are simply dropped. // This string is a type URL. std::unique_ptr> request_queue_; + + // Envoy's dependency ordering. + std::list subscriptions_; + const envoy::config::core::v3::ApiVersion transport_api_version_; Event::Dispatcher& dispatcher_; diff --git a/source/common/config/grpc_stream.h b/source/common/config/grpc_stream.h index f6673402df4f..07fc01061493 100644 --- a/source/common/config/grpc_stream.h +++ b/source/common/config/grpc_stream.h @@ -71,7 +71,11 @@ class GrpcStream : public Grpc::AsyncStreamCallbacks, bool grpcStreamAvailable() const { return stream_ != nullptr; } - void sendMessage(const RequestProto& request) { stream_->sendMessage(request, false); } + void sendMessage(const RequestProto& request) { + if (stream_ != nullptr) { + stream_->sendMessage(request, false); + } + } // Grpc::AsyncStreamCallbacks void onCreateInitialMetadata(Http::RequestHeaderMap& metadata) override { diff --git a/source/common/grpc/async_client_impl.cc b/source/common/grpc/async_client_impl.cc index 6d1152ef9f45..ee8245b1cf66 100644 --- a/source/common/grpc/async_client_impl.cc +++ b/source/common/grpc/async_client_impl.cc @@ -22,6 +22,9 @@ AsyncClientImpl::AsyncClientImpl(Upstream::ClusterManager& cm, AsyncClientImpl::~AsyncClientImpl() { while (!active_streams_.empty()) { + FANCY_LOG(info, "destroying active stream {} of async client imp {}", + static_cast(active_streams_.front().get()), + static_cast(this)); active_streams_.front()->resetStream(); } } diff --git a/source/common/grpc/typed_async_client.h b/source/common/grpc/typed_async_client.h index 2905db8f345a..d04d98c3bd73 100644 --- a/source/common/grpc/typed_async_client.h +++ b/source/common/grpc/typed_async_client.h @@ -47,7 +47,10 @@ template class AsyncStream /* : public RawAsyncStream */ { Internal::sendMessageUntyped(stream_, std::move(request), end_stream); } void closeStream() { stream_->closeStream(); } - void resetStream() { stream_->resetStream(); } + void resetStream() { + stream_->resetStream(); + stream_ = nullptr; + } bool isAboveWriteBufferHighWatermark() const { return stream_->isAboveWriteBufferHighWatermark(); } diff --git a/source/server/server.cc b/source/server/server.cc index 9f41a126d67f..3828ad7642d4 100644 --- a/source/server/server.cc +++ b/source/server/server.cc @@ -79,6 +79,7 @@ InstanceImpl::InstanceImpl( : absl::nullopt, watermark_factory)), dispatcher_(api_->allocateDispatcher("main_thread")), + dispatcher_shutdown_helper_(*dispatcher_), singleton_manager_(new Singleton::ManagerImpl(api_->threadFactory())), handler_(new ConnectionHandlerImpl(*dispatcher_, absl::nullopt)), listener_component_factory_(*this), worker_factory_(thread_local_, *api_, hooks), @@ -139,7 +140,7 @@ InstanceImpl::~InstanceImpl() { ENVOY_LOG(debug, "destroying listener manager"); listener_manager_.reset(); ENVOY_LOG(debug, "destroyed listener manager"); - dispatcher_->shutdown(); + // dispatcher_->shutdown(); } Upstream::ClusterManager& InstanceImpl::clusterManager() { diff --git a/source/server/server.h b/source/server/server.h index cea2e6dff812..ec0725104163 100644 --- a/source/server/server.h +++ b/source/server/server.h @@ -294,6 +294,12 @@ class InstanceImpl final : Logger::Loggable, registerCallback(Stage stage, StageCallbackWithCompletion callback) override; private: + struct DispatcherShutdownHelper { + + DispatcherShutdownHelper(Event::Dispatcher& dispatcher) : dispatcher_(dispatcher) {} + ~DispatcherShutdownHelper() { dispatcher_.shutdown(); } + Event::Dispatcher& dispatcher_; + }; ProtobufTypes::MessagePtr dumpBootstrapConfig(); void flushStatsInternal(); void updateServerStats(); @@ -342,6 +348,7 @@ class InstanceImpl final : Logger::Loggable, Random::RandomGeneratorPtr random_generator_; Api::ApiPtr api_; Event::DispatcherPtr dispatcher_; + DispatcherShutdownHelper dispatcher_shutdown_helper_; std::unique_ptr admin_; Singleton::ManagerPtr singleton_manager_; Network::ConnectionHandlerPtr handler_; From 31fc603b28f7926b1f5d5f655e094c8887aabdf6 Mon Sep 17 00:00:00 2001 From: Yuchen Dai Date: Wed, 17 Feb 2021 03:49:54 -0800 Subject: [PATCH 33/40] DispatcherShutdownTest revert check deletable object empty. const and test Signed-off-by: Yuchen Dai --- include/envoy/event/dispatcher.h | 6 +- .../envoy/event/dispatcher_thread_deletable.h | 1 + source/common/event/dispatcher_impl.cc | 70 +++------ source/common/event/dispatcher_impl.h | 4 +- source/common/upstream/upstream_impl.cc | 2 +- source/server/server.cc | 3 +- source/server/server.h | 7 - test/common/event/dispatcher_impl_test.cc | 142 +++++++++++++----- test/mocks/event/mocks.h | 2 +- test/mocks/event/wrapped_dispatcher.h | 2 +- 10 files changed, 140 insertions(+), 99 deletions(-) diff --git a/include/envoy/event/dispatcher.h b/include/envoy/event/dispatcher.h index 90f405d8d00e..276de18fdd3c 100644 --- a/include/envoy/event/dispatcher.h +++ b/include/envoy/event/dispatcher.h @@ -263,9 +263,9 @@ class Dispatcher : public DispatcherBase { /** * Post the deletable to this dispatcher. The deletable objects are guaranteed to be destroyed on - * the dispatcher's thread before the dispatcher is destroyed. This is safe cross thread. + * the dispatcher's thread before dispatcher destroy. This is safe cross thread. */ - virtual void deleteInDispatcherThread(DispatcherThreadDeletablePtr deletable) PURE; + virtual void deleteInDispatcherThread(DispatcherThreadDeletableConstPtr deletable) PURE; /** * Runs the event loop. This will not return until exit() is called either from within a callback @@ -296,7 +296,7 @@ class Dispatcher : public DispatcherBase { virtual void updateApproximateMonotonicTime() PURE; /** - * Shutdown the dispatcher by clearing the dispatcher's thread deletable objects. + * Shutdown the dispatcher by clear dispatcher thread deletable. */ virtual void shutdown() PURE; }; diff --git a/include/envoy/event/dispatcher_thread_deletable.h b/include/envoy/event/dispatcher_thread_deletable.h index 1416456603d0..71eb71425bd0 100644 --- a/include/envoy/event/dispatcher_thread_deletable.h +++ b/include/envoy/event/dispatcher_thread_deletable.h @@ -16,6 +16,7 @@ class DispatcherThreadDeletable { }; using DispatcherThreadDeletablePtr = std::unique_ptr; +using DispatcherThreadDeletableConstPtr = std::unique_ptr; } // namespace Event } // namespace Envoy diff --git a/source/common/event/dispatcher_impl.cc b/source/common/event/dispatcher_impl.cc index e7c05f0784a3..9748f82bb433 100644 --- a/source/common/event/dispatcher_impl.cc +++ b/source/common/event/dispatcher_impl.cc @@ -77,12 +77,10 @@ DispatcherImpl::DispatcherImpl(const std::string& name, Api::Api& api, } DispatcherImpl::~DispatcherImpl() { - // TODO(lambdai): call shutdown() in tests and assert shutdown_called_. ENVOY_LOG(debug, "destroying dispatcher {}", name_); FatalErrorHandler::removeFatalErrorHandler(*this); - ASSERT(deletables_in_dispatcher_thread_.empty(), - fmt::format("{} dispatcher contains {} dispatcher local deletables after shutdown.", - run_tid_.getId(), deletables_in_dispatcher_thread_.size())); + // TODO(lambdai): Resolve https://github.com/envoyproxy/envoy/issues/15072 and enable + // ASSERT(deletable_in_dispatcher_thread_.empty()) } void DispatcherImpl::registerWatchdog(const Server::WatchDogSharedPtr& watchdog, @@ -274,7 +272,7 @@ void DispatcherImpl::post(std::function callback) { } } -void DispatcherImpl::deleteInDispatcherThread(DispatcherThreadDeletablePtr deletable) { +void DispatcherImpl::deleteInDispatcherThread(DispatcherThreadDeletableConstPtr deletable) { bool need_schedule; { Thread::LockGuard lock(thread_local_deletable_lock_); @@ -303,50 +301,30 @@ MonotonicTime DispatcherImpl::approximateMonotonicTime() const { } void DispatcherImpl::shutdown() { + // TODO(lambdai): Resolve https://github.com/envoyproxy/envoy/issues/15072 and loop delete 3 lists + // until all lists are empty. ASSERT(isThreadSafe()); + auto deferred_deletables_size = current_to_delete_->size(); + std::list>::size_type post_callbacks_size; + { + Thread::LockGuard lock(post_lock_); + post_callbacks_size = post_callbacks_.size(); + } - bool delete_again = true; - int round = 0; - while (delete_again) { - delete_again = false; - round++; - auto deferred_deletables_size = current_to_delete_->size(); - delete_again |= deferred_deletables_size > 0; - clearDeferredDeleteList(); - - std::list> callbacks; - { - Thread::LockGuard lock(post_lock_); - callbacks = std::move(post_callbacks_); - } - - auto post_callbacks_size = callbacks.size(); - while (!callbacks.empty()) { - // Don't invoke callbacks when the dispatcher is shutting down. - callbacks.pop_front(); - } - delete_again |= post_callbacks_size > 0; - - std::list local_deletables; - { - Thread::LockGuard lock(thread_local_deletable_lock_); - local_deletables = std::move(deletables_in_dispatcher_thread_); - } - auto thread_local_deletables_size = local_deletables.size(); - while (!local_deletables.empty()) { - local_deletables.pop_front(); - } - - delete_again |= thread_local_deletables_size > 0; - ENVOY_LOG(debug, - "Round {}: destroyed {} deferred deletables, {} post callbacks and {} thread local " - "deletables in {}", - round, deferred_deletables_size, post_callbacks_size, thread_local_deletables_size, - __FUNCTION__); + std::list local_deletables; + { + Thread::LockGuard lock(thread_local_deletable_lock_); + local_deletables = std::move(deletables_in_dispatcher_thread_); + } + auto thread_local_deletables_size = local_deletables.size(); + while (!local_deletables.empty()) { + local_deletables.pop_front(); } - ASSERT(!shutdown_called_); - shutdown_called_ = true; + ENVOY_LOG( + debug, + "{} destroyed {} thread local objects. Peek {} deferred deletables, {} post callbacks. ", + __FUNCTION__, deferred_deletables_size, post_callbacks_size, thread_local_deletables_size); } void DispatcherImpl::updateApproximateMonotonicTime() { updateApproximateMonotonicTimeInternal(); } @@ -356,7 +334,7 @@ void DispatcherImpl::updateApproximateMonotonicTimeInternal() { } void DispatcherImpl::runThreadLocalDelete() { - std::list to_be_delete; + std::list to_be_delete; { Thread::LockGuard lock(thread_local_deletable_lock_); to_be_delete = std::move(deletables_in_dispatcher_thread_); diff --git a/source/common/event/dispatcher_impl.h b/source/common/event/dispatcher_impl.h index 5c9568ae076e..a4eec3790b25 100644 --- a/source/common/event/dispatcher_impl.h +++ b/source/common/event/dispatcher_impl.h @@ -86,7 +86,7 @@ class DispatcherImpl : Logger::Loggable, void exit() override; SignalEventPtr listenForSignal(signal_t signal_num, SignalCb cb) override; void post(std::function callback) override; - void deleteInDispatcherThread(DispatcherThreadDeletablePtr deletable) override; + void deleteInDispatcherThread(DispatcherThreadDeletableConstPtr deletable) override; void run(RunType type) override; Buffer::WatermarkFactory& getWatermarkFactory() override { return *buffer_factory_; } void pushTrackedObject(const ScopeTrackedObject* object) override; @@ -153,7 +153,7 @@ class DispatcherImpl : Logger::Loggable, SchedulableCallbackPtr thread_local_delete_cb_; Thread::MutexBasicLockable thread_local_deletable_lock_; // `deletables_in_dispatcher_thread` must be destroyed last to allow other callbacks populate. - std::list + std::list deletables_in_dispatcher_thread_ ABSL_GUARDED_BY(thread_local_deletable_lock_); bool shutdown_called_{false}; diff --git a/source/common/upstream/upstream_impl.cc b/source/common/upstream/upstream_impl.cc index a34b69531f25..adee065006e1 100644 --- a/source/common/upstream/upstream_impl.cc +++ b/source/common/upstream/upstream_impl.cc @@ -927,7 +927,7 @@ ClusterImplBase::ClusterImplBase( [&dispatcher](const ClusterInfoImpl* self) { ENVOY_LOG(debug, "Schedule destroy cluster info {}", self->name()); dispatcher.deleteInDispatcherThread( - std::unique_ptr(const_cast(self))); + std::unique_ptr(self)); }); if ((info_->features() & ClusterInfoImpl::Features::USE_ALPN) && diff --git a/source/server/server.cc b/source/server/server.cc index 3828ad7642d4..9f41a126d67f 100644 --- a/source/server/server.cc +++ b/source/server/server.cc @@ -79,7 +79,6 @@ InstanceImpl::InstanceImpl( : absl::nullopt, watermark_factory)), dispatcher_(api_->allocateDispatcher("main_thread")), - dispatcher_shutdown_helper_(*dispatcher_), singleton_manager_(new Singleton::ManagerImpl(api_->threadFactory())), handler_(new ConnectionHandlerImpl(*dispatcher_, absl::nullopt)), listener_component_factory_(*this), worker_factory_(thread_local_, *api_, hooks), @@ -140,7 +139,7 @@ InstanceImpl::~InstanceImpl() { ENVOY_LOG(debug, "destroying listener manager"); listener_manager_.reset(); ENVOY_LOG(debug, "destroyed listener manager"); - // dispatcher_->shutdown(); + dispatcher_->shutdown(); } Upstream::ClusterManager& InstanceImpl::clusterManager() { diff --git a/source/server/server.h b/source/server/server.h index ec0725104163..cea2e6dff812 100644 --- a/source/server/server.h +++ b/source/server/server.h @@ -294,12 +294,6 @@ class InstanceImpl final : Logger::Loggable, registerCallback(Stage stage, StageCallbackWithCompletion callback) override; private: - struct DispatcherShutdownHelper { - - DispatcherShutdownHelper(Event::Dispatcher& dispatcher) : dispatcher_(dispatcher) {} - ~DispatcherShutdownHelper() { dispatcher_.shutdown(); } - Event::Dispatcher& dispatcher_; - }; ProtobufTypes::MessagePtr dumpBootstrapConfig(); void flushStatsInternal(); void updateServerStats(); @@ -348,7 +342,6 @@ class InstanceImpl final : Logger::Loggable, Random::RandomGeneratorPtr random_generator_; Api::ApiPtr api_; Event::DispatcherPtr dispatcher_; - DispatcherShutdownHelper dispatcher_shutdown_helper_; std::unique_ptr admin_; Singleton::ManagerPtr singleton_manager_; Network::ConnectionHandlerPtr handler_; diff --git a/test/common/event/dispatcher_impl_test.cc b/test/common/event/dispatcher_impl_test.cc index a0b5785d134e..eb93fed00973 100644 --- a/test/common/event/dispatcher_impl_test.cc +++ b/test/common/event/dispatcher_impl_test.cc @@ -33,30 +33,6 @@ using testing::Return; namespace Envoy { namespace Event { namespace { -class Destroyable : public DeferredDeletable, public DispatcherThreadDeletable { -public: - Destroyable(Dispatcher& dispatcher, int life, ReadyWatcher& watcher) - : dispatcher_(dispatcher), life_(life), watcher_(watcher) {} - ~Destroyable() { - - if (life_ != 0) { - if ((life_ % 3) == 0) { - dispatcher_.post([obj = std::make_shared( - dispatcher_, life_ - 1, watcher_)]() mutable { obj.reset(); }); - } else if ((life_ % 3) == 1) { - dispatcher_.deleteInDispatcherThread( - std::make_unique(dispatcher_, life_ - 1, watcher_)); - } else { - dispatcher_.deferredDelete(std::make_unique(dispatcher_, life_ - 1, watcher_)); - } - } else { - watcher_.ready(); - } - } - Dispatcher& dispatcher_; - uint32_t life_; - ReadyWatcher& watcher_; -}; class RunOnDelete { public: @@ -264,6 +240,15 @@ class TestDeferredDeletable : public DeferredDeletable { std::function on_destroy_; }; +class TestDispatcherThreadDeletable : public DispatcherThreadDeletable { +public: + TestDispatcherThreadDeletable(std::function on_destroy) : on_destroy_(on_destroy) {} + ~TestDispatcherThreadDeletable() override { on_destroy_(); } + +private: + std::function on_destroy_; +}; + TEST(DeferredDeleteTest, DeferredDelete) { InSequence s; Api::ApiPtr api = Api::createApiForTest(); @@ -382,6 +367,7 @@ class DispatcherImplTest : public testing::Test { cv_.wait(mu_); } } + NiceMock scope_; // Used in InitializeStats, must outlive dispatcher_->exit(). Api::ApiPtr api_; Thread::ThreadPtr dispatcher_thread_; @@ -465,18 +451,6 @@ TEST_F(DispatcherImplTest, PostExecuteAndDestructOrder) { } } -TEST(DispatcherShutdownTest, ShutdownCleanupAllDestroyableObjects) { - auto api = Api::createApiForTest(); - auto dispatcher = api->allocateDispatcher("test_thread"); - ReadyWatcher shutdown_watcher; - dispatcher->deferredDelete(std::make_unique(*dispatcher, 10, shutdown_watcher)); - - EXPECT_CALL(shutdown_watcher, ready()); - dispatcher->shutdown(); - // Need to verify before dispatcher is destroyed. - testing::Mock::VerifyAndClearExpectations(&shutdown_watcher); -} - // Ensure that there is no deadlock related to calling a posted callback, or // destructing a closure when finished calling it. TEST_F(DispatcherImplTest, RunPostCallbacksLocking) { @@ -517,6 +491,102 @@ TEST_F(DispatcherImplTest, RunPostCallbacksLocking) { } } +TEST_F(DispatcherImplTest, DispatcherThreadDeleted) { + dispatcher_->deleteInDispatcherThread(std::make_unique( + [this, id = api_->threadFactory().currentThreadId()]() { + ASSERT(id != api_->threadFactory().currentThreadId()); + { + Thread::LockGuard lock(mu_); + ASSERT(!work_finished_); + work_finished_ = true; + } + cv_.notifyOne(); + })); + + Thread::LockGuard lock(mu_); + while (!work_finished_) { + cv_.wait(mu_); + } +} + +TEST_F(DispatcherImplTest, DispatcherThreadDeletedAtNextCycle) { + dispatcher_->deleteInDispatcherThread(std::make_unique( + [this, id = api_->threadFactory().currentThreadId()]() { + ASSERT(id != api_->threadFactory().currentThreadId()); + { + Thread::LockGuard lock(mu_); + ASSERT(!work_finished_); + work_finished_ = true; + } + cv_.notifyOne(); + })); + + Thread::LockGuard lock(mu_); + while (!work_finished_) { + cv_.wait(mu_); + } +} + +class DispatcherShutdownTest : public testing::Test { +protected: + DispatcherShutdownTest() + : api_(Api::createApiForTest()), dispatcher_(api_->allocateDispatcher("test_thread")) {} + + Api::ApiPtr api_; + DispatcherPtr dispatcher_; +}; + +TEST_F(DispatcherShutdownTest, ShutdownClearThreadLocalDeletables) { + ReadyWatcher watcher; + + dispatcher_->deleteInDispatcherThread( + std::make_unique([&watcher]() { watcher.ready(); })); + EXPECT_CALL(watcher, ready()); + dispatcher_->shutdown(); +} + +TEST_F(DispatcherShutdownTest, ShutdownDoesnotClearDeferredListOrPostCallback) { + ReadyWatcher watcher; + ReadyWatcher deferred_watcher; + ReadyWatcher post_watcher; + + { + InSequence s; + + dispatcher_->deferredDelete(std::make_unique( + [&deferred_watcher]() { deferred_watcher.ready(); })); + dispatcher_->post([&post_watcher]() { post_watcher.ready(); }); + dispatcher_->deleteInDispatcherThread( + std::make_unique([&watcher]() { watcher.ready(); })); + EXPECT_CALL(watcher, ready()); + dispatcher_->shutdown(); + + ::testing::Mock::VerifyAndClearExpectations(&watcher); + EXPECT_CALL(deferred_watcher, ready()); + + dispatcher_.reset(); + ::testing::Mock::VerifyAndClearExpectations(&deferred_watcher); + ::testing::Mock::VerifyAndClearExpectations(&post_watcher); + } +} + +TEST_F(DispatcherShutdownTest, DestroyClearAllList) { + ReadyWatcher watcher; + ReadyWatcher deferred_watcher; + dispatcher_->deferredDelete( + std::make_unique([&deferred_watcher]() { deferred_watcher.ready(); })); + dispatcher_->deleteInDispatcherThread( + std::make_unique([&watcher]() { watcher.ready(); })); + { + InSequence s; + EXPECT_CALL(deferred_watcher, ready()); + EXPECT_CALL(watcher, ready()); + dispatcher_.reset(); + ::testing::Mock::VerifyAndClearExpectations(&deferred_watcher); + ::testing::Mock::VerifyAndClearExpectations(&watcher); + } +} + TEST_F(DispatcherImplTest, Timer) { timerTest([](Timer& timer) { timer.enableTimer(std::chrono::milliseconds(0)); }); timerTest([](Timer& timer) { timer.enableTimer(std::chrono::milliseconds(50)); }); diff --git a/test/mocks/event/mocks.h b/test/mocks/event/mocks.h index 9eda2ef636a9..0c107fa8a00b 100644 --- a/test/mocks/event/mocks.h +++ b/test/mocks/event/mocks.h @@ -146,7 +146,7 @@ class MockDispatcher : public Dispatcher { MOCK_METHOD(void, exit, ()); MOCK_METHOD(SignalEvent*, listenForSignal_, (signal_t signal_num, SignalCb cb)); MOCK_METHOD(void, post, (std::function callback)); - MOCK_METHOD(void, deleteInDispatcherThread, (DispatcherThreadDeletablePtr deletable)); + MOCK_METHOD(void, deleteInDispatcherThread, (DispatcherThreadDeletableConstPtr deletable)); MOCK_METHOD(void, run, (RunType type)); MOCK_METHOD(void, pushTrackedObject, (const ScopeTrackedObject* object)); MOCK_METHOD(void, popTrackedObject, (const ScopeTrackedObject* expected_object)); diff --git a/test/mocks/event/wrapped_dispatcher.h b/test/mocks/event/wrapped_dispatcher.h index 18f99ac58c5f..780480902035 100644 --- a/test/mocks/event/wrapped_dispatcher.h +++ b/test/mocks/event/wrapped_dispatcher.h @@ -101,7 +101,7 @@ class WrappedDispatcher : public Dispatcher { void post(std::function callback) override { impl_.post(std::move(callback)); } - void deleteInDispatcherThread(DispatcherThreadDeletablePtr deletable) override { + void deleteInDispatcherThread(DispatcherThreadDeletableConstPtr deletable) override { impl_.deleteInDispatcherThread(std::move(deletable)); } From f9cd68fe76f9fd9e5c5dbe12329e510e86f6218c Mon Sep 17 00:00:00 2001 From: Yuchen Dai Date: Wed, 17 Feb 2021 14:20:34 -0800 Subject: [PATCH 34/40] test, log cleanup Signed-off-by: Yuchen Dai --- .../envoy/event/dispatcher_thread_deletable.h | 1 - source/common/config/grpc_stream.h | 6 +-- source/common/event/dispatcher_impl.cc | 11 ++++-- source/common/grpc/async_client_impl.cc | 3 -- test/common/event/dispatcher_impl_test.cc | 37 +++++++++---------- 5 files changed, 27 insertions(+), 31 deletions(-) diff --git a/include/envoy/event/dispatcher_thread_deletable.h b/include/envoy/event/dispatcher_thread_deletable.h index 71eb71425bd0..bf5b1808e0e3 100644 --- a/include/envoy/event/dispatcher_thread_deletable.h +++ b/include/envoy/event/dispatcher_thread_deletable.h @@ -15,7 +15,6 @@ class DispatcherThreadDeletable { virtual ~DispatcherThreadDeletable() = default; }; -using DispatcherThreadDeletablePtr = std::unique_ptr; using DispatcherThreadDeletableConstPtr = std::unique_ptr; } // namespace Event diff --git a/source/common/config/grpc_stream.h b/source/common/config/grpc_stream.h index 07fc01061493..fb2dd1a30e66 100644 --- a/source/common/config/grpc_stream.h +++ b/source/common/config/grpc_stream.h @@ -72,9 +72,9 @@ class GrpcStream : public Grpc::AsyncStreamCallbacks, bool grpcStreamAvailable() const { return stream_ != nullptr; } void sendMessage(const RequestProto& request) { - if (stream_ != nullptr) { - stream_->sendMessage(request, false); - } + ASSERT(stream_, absl::StrCat("sending message to invalid grpc stream for method ", + service_method_.DebugString())); + stream_->sendMessage(request, false); } // Grpc::AsyncStreamCallbacks diff --git a/source/common/event/dispatcher_impl.cc b/source/common/event/dispatcher_impl.cc index 9748f82bb433..fc5188fab7d6 100644 --- a/source/common/event/dispatcher_impl.cc +++ b/source/common/event/dispatcher_impl.cc @@ -278,7 +278,8 @@ void DispatcherImpl::deleteInDispatcherThread(DispatcherThreadDeletableConstPtr Thread::LockGuard lock(thread_local_deletable_lock_); need_schedule = deletables_in_dispatcher_thread_.empty(); deletables_in_dispatcher_thread_.emplace_back(std::move(deletable)); - ASSERT(!shutdown_called_, "inserted after shutdown"); + // TODO(lambdai): Enable below after https://github.com/envoyproxy/envoy/issues/15072 + // ASSERT(!shutdown_called_, "inserted after shutdown"); } if (need_schedule) { @@ -301,8 +302,9 @@ MonotonicTime DispatcherImpl::approximateMonotonicTime() const { } void DispatcherImpl::shutdown() { - // TODO(lambdai): Resolve https://github.com/envoyproxy/envoy/issues/15072 and loop delete 3 lists - // until all lists are empty. + // TODO(lambdai): Resolve https://github.com/envoyproxy/envoy/issues/15072 and loop delete below + // below 3 lists until all lists are empty. The 3 lists are list of deferred delete objects, post + // callbacks and dispatcher thread deletable objects. ASSERT(isThreadSafe()); auto deferred_deletables_size = current_to_delete_->size(); std::list>::size_type post_callbacks_size; @@ -320,7 +322,8 @@ void DispatcherImpl::shutdown() { while (!local_deletables.empty()) { local_deletables.pop_front(); } - + ASSERT(!shutdown_called_); + shutdown_called_ = true; ENVOY_LOG( debug, "{} destroyed {} thread local objects. Peek {} deferred deletables, {} post callbacks. ", diff --git a/source/common/grpc/async_client_impl.cc b/source/common/grpc/async_client_impl.cc index ee8245b1cf66..6d1152ef9f45 100644 --- a/source/common/grpc/async_client_impl.cc +++ b/source/common/grpc/async_client_impl.cc @@ -22,9 +22,6 @@ AsyncClientImpl::AsyncClientImpl(Upstream::ClusterManager& cm, AsyncClientImpl::~AsyncClientImpl() { while (!active_streams_.empty()) { - FANCY_LOG(info, "destroying active stream {} of async client imp {}", - static_cast(active_streams_.front().get()), - static_cast(this)); active_streams_.front()->resetStream(); } } diff --git a/test/common/event/dispatcher_impl_test.cc b/test/common/event/dispatcher_impl_test.cc index eb93fed00973..684ac378ae84 100644 --- a/test/common/event/dispatcher_impl_test.cc +++ b/test/common/event/dispatcher_impl_test.cc @@ -509,22 +509,24 @@ TEST_F(DispatcherImplTest, DispatcherThreadDeleted) { } } -TEST_F(DispatcherImplTest, DispatcherThreadDeletedAtNextCycle) { - dispatcher_->deleteInDispatcherThread(std::make_unique( - [this, id = api_->threadFactory().currentThreadId()]() { - ASSERT(id != api_->threadFactory().currentThreadId()); - { - Thread::LockGuard lock(mu_); - ASSERT(!work_finished_); - work_finished_ = true; - } - cv_.notifyOne(); - })); - - Thread::LockGuard lock(mu_); - while (!work_finished_) { - cv_.wait(mu_); +TEST(DispatcherThreadDeletedImplTest, DispatcherThreadDeletedAtNextCycle) { + Api::ApiPtr api_(Api::createApiForTest()); + DispatcherPtr dispatcher(api_->allocateDispatcher("test_thread")); + std::vector> watchers; + for (int i = 0; i < 3; ++i) { + watchers.push_back(std::make_unique()); } + dispatcher->deleteInDispatcherThread( + std::make_unique([&watchers]() { watchers[0]->ready(); })); + EXPECT_CALL(*watchers[0], ready()); + dispatcher->run(Event::Dispatcher::RunType::NonBlock); + dispatcher->deleteInDispatcherThread( + std::make_unique([&watchers]() { watchers[1]->ready(); })); + dispatcher->deleteInDispatcherThread( + std::make_unique([&watchers]() { watchers[2]->ready(); })); + EXPECT_CALL(*watchers[1], ready()); + EXPECT_CALL(*watchers[2], ready()); + dispatcher->run(Event::Dispatcher::RunType::NonBlock); } class DispatcherShutdownTest : public testing::Test { @@ -563,10 +565,7 @@ TEST_F(DispatcherShutdownTest, ShutdownDoesnotClearDeferredListOrPostCallback) { ::testing::Mock::VerifyAndClearExpectations(&watcher); EXPECT_CALL(deferred_watcher, ready()); - dispatcher_.reset(); - ::testing::Mock::VerifyAndClearExpectations(&deferred_watcher); - ::testing::Mock::VerifyAndClearExpectations(&post_watcher); } } @@ -582,8 +581,6 @@ TEST_F(DispatcherShutdownTest, DestroyClearAllList) { EXPECT_CALL(deferred_watcher, ready()); EXPECT_CALL(watcher, ready()); dispatcher_.reset(); - ::testing::Mock::VerifyAndClearExpectations(&deferred_watcher); - ::testing::Mock::VerifyAndClearExpectations(&watcher); } } From 01a6557fd8bbea609c0fa0ff450aa43a70d90f12 Mon Sep 17 00:00:00 2001 From: Yuchen Dai Date: Wed, 17 Feb 2021 14:24:46 -0800 Subject: [PATCH 35/40] revert source/common/config/grpc_mux_impl.h Signed-off-by: Yuchen Dai --- source/common/config/grpc_mux_impl.h | 7 +++---- 1 file changed, 3 insertions(+), 4 deletions(-) diff --git a/source/common/config/grpc_mux_impl.h b/source/common/config/grpc_mux_impl.h index e89c1d4b0886..4b46dbb67317 100644 --- a/source/common/config/grpc_mux_impl.h +++ b/source/common/config/grpc_mux_impl.h @@ -161,14 +161,13 @@ class GrpcMuxImpl : public GrpcMux, absl::node_hash_map> api_state_; + // Envoy's dependency ordering. + std::list subscriptions_; + // A queue to store requests while rate limited. Note that when requests cannot be sent due to the // gRPC stream being down, this queue does not store them; rather, they are simply dropped. // This string is a type URL. std::unique_ptr> request_queue_; - - // Envoy's dependency ordering. - std::list subscriptions_; - const envoy::config::core::v3::ApiVersion transport_api_version_; Event::Dispatcher& dispatcher_; From eb0198972a9c12be64330220c44fe9510255657c Mon Sep 17 00:00:00 2001 From: Yuchen Dai Date: Wed, 17 Feb 2021 14:27:41 -0800 Subject: [PATCH 36/40] revert source/common/grpc/typed_async_client.h Signed-off-by: Yuchen Dai --- source/common/grpc/typed_async_client.h | 5 +---- 1 file changed, 1 insertion(+), 4 deletions(-) diff --git a/source/common/grpc/typed_async_client.h b/source/common/grpc/typed_async_client.h index d04d98c3bd73..2905db8f345a 100644 --- a/source/common/grpc/typed_async_client.h +++ b/source/common/grpc/typed_async_client.h @@ -47,10 +47,7 @@ template class AsyncStream /* : public RawAsyncStream */ { Internal::sendMessageUntyped(stream_, std::move(request), end_stream); } void closeStream() { stream_->closeStream(); } - void resetStream() { - stream_->resetStream(); - stream_ = nullptr; - } + void resetStream() { stream_->resetStream(); } bool isAboveWriteBufferHighWatermark() const { return stream_->isAboveWriteBufferHighWatermark(); } From 32381e63378e17a1f2fca2066c12fbc7fe4e6e06 Mon Sep 17 00:00:00 2001 From: Yuchen Dai Date: Wed, 17 Feb 2021 20:35:04 -0800 Subject: [PATCH 37/40] revert assert on nullptr Signed-off-by: Yuchen Dai --- source/common/config/grpc_stream.h | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/source/common/config/grpc_stream.h b/source/common/config/grpc_stream.h index fb2dd1a30e66..560cad3a43a4 100644 --- a/source/common/config/grpc_stream.h +++ b/source/common/config/grpc_stream.h @@ -72,8 +72,8 @@ class GrpcStream : public Grpc::AsyncStreamCallbacks, bool grpcStreamAvailable() const { return stream_ != nullptr; } void sendMessage(const RequestProto& request) { - ASSERT(stream_, absl::StrCat("sending message to invalid grpc stream for method ", - service_method_.DebugString())); + ASSERT(stream_ != nullptr, absl::StrCat("sending message to invalid grpc stream for method ", + service_method_.DebugString())); stream_->sendMessage(request, false); } From 19309cb29090eb8920b41f05840088e8077a4f4b Mon Sep 17 00:00:00 2001 From: Yuchen Dai Date: Wed, 17 Feb 2021 23:42:41 -0800 Subject: [PATCH 38/40] call reserve() to massage clangtidy Signed-off-by: Yuchen Dai --- test/common/event/dispatcher_impl_test.cc | 1 + 1 file changed, 1 insertion(+) diff --git a/test/common/event/dispatcher_impl_test.cc b/test/common/event/dispatcher_impl_test.cc index 684ac378ae84..345305a35b63 100644 --- a/test/common/event/dispatcher_impl_test.cc +++ b/test/common/event/dispatcher_impl_test.cc @@ -513,6 +513,7 @@ TEST(DispatcherThreadDeletedImplTest, DispatcherThreadDeletedAtNextCycle) { Api::ApiPtr api_(Api::createApiForTest()); DispatcherPtr dispatcher(api_->allocateDispatcher("test_thread")); std::vector> watchers; + watchers.reserve(3); for (int i = 0; i < 3; ++i) { watchers.push_back(std::make_unique()); } From 171b6d4144c7caafcbd9cb2d55093b07956834b3 Mon Sep 17 00:00:00 2001 From: Yuchen Dai Date: Mon, 22 Feb 2021 16:44:53 -0800 Subject: [PATCH 39/40] trace level and delete assert nullptr before use Signed-off-by: Yuchen Dai --- source/common/config/grpc_stream.h | 2 -- source/common/event/dispatcher_impl.cc | 2 +- source/common/upstream/upstream_impl.cc | 2 +- 3 files changed, 2 insertions(+), 4 deletions(-) diff --git a/source/common/config/grpc_stream.h b/source/common/config/grpc_stream.h index 560cad3a43a4..a887c98e343d 100644 --- a/source/common/config/grpc_stream.h +++ b/source/common/config/grpc_stream.h @@ -72,8 +72,6 @@ class GrpcStream : public Grpc::AsyncStreamCallbacks, bool grpcStreamAvailable() const { return stream_ != nullptr; } void sendMessage(const RequestProto& request) { - ASSERT(stream_ != nullptr, absl::StrCat("sending message to invalid grpc stream for method ", - service_method_.DebugString())); stream_->sendMessage(request, false); } diff --git a/source/common/event/dispatcher_impl.cc b/source/common/event/dispatcher_impl.cc index fc5188fab7d6..ef68c020b190 100644 --- a/source/common/event/dispatcher_impl.cc +++ b/source/common/event/dispatcher_impl.cc @@ -325,7 +325,7 @@ void DispatcherImpl::shutdown() { ASSERT(!shutdown_called_); shutdown_called_ = true; ENVOY_LOG( - debug, + trace, "{} destroyed {} thread local objects. Peek {} deferred deletables, {} post callbacks. ", __FUNCTION__, deferred_deletables_size, post_callbacks_size, thread_local_deletables_size); } diff --git a/source/common/upstream/upstream_impl.cc b/source/common/upstream/upstream_impl.cc index fc449028dbc6..05f9d85460ef 100644 --- a/source/common/upstream/upstream_impl.cc +++ b/source/common/upstream/upstream_impl.cc @@ -929,7 +929,7 @@ ClusterImplBase::ClusterImplBase( std::move(socket_matcher), std::move(stats_scope), added_via_api, factory_context), [&dispatcher](const ClusterInfoImpl* self) { - ENVOY_LOG(debug, "Schedule destroy cluster info {}", self->name()); + ENVOY_LOG(trace, "Schedule destroy cluster info {}", self->name()); dispatcher.deleteInDispatcherThread( std::unique_ptr(self)); }); From d1bb2d54d0541d467ab560f352675a2dd6923454 Mon Sep 17 00:00:00 2001 From: Yuchen Dai Date: Mon, 22 Feb 2021 17:30:13 -0800 Subject: [PATCH 40/40] fix format Signed-off-by: Yuchen Dai --- source/common/config/grpc_stream.h | 4 +--- 1 file changed, 1 insertion(+), 3 deletions(-) diff --git a/source/common/config/grpc_stream.h b/source/common/config/grpc_stream.h index a887c98e343d..f6673402df4f 100644 --- a/source/common/config/grpc_stream.h +++ b/source/common/config/grpc_stream.h @@ -71,9 +71,7 @@ class GrpcStream : public Grpc::AsyncStreamCallbacks, bool grpcStreamAvailable() const { return stream_ != nullptr; } - void sendMessage(const RequestProto& request) { - stream_->sendMessage(request, false); - } + void sendMessage(const RequestProto& request) { stream_->sendMessage(request, false); } // Grpc::AsyncStreamCallbacks void onCreateInitialMetadata(Http::RequestHeaderMap& metadata) override {