diff --git a/include/envoy/server/instance.h b/include/envoy/server/instance.h index 576cf8c4eef4..c455c76681d3 100644 --- a/include/envoy/server/instance.h +++ b/include/envoy/server/instance.h @@ -152,6 +152,11 @@ class Instance { */ virtual void shutdown() PURE; + /** + * @return whether the shutdown method has been called. + */ + virtual bool isShutdown() PURE; + /** * Shutdown the server's admin processing. This includes the admin API, stat flushing, etc. */ diff --git a/source/server/config_validation/server.h b/source/server/config_validation/server.h index 03d0b9538971..dfbab3bb2bd0 100644 --- a/source/server/config_validation/server.h +++ b/source/server/config_validation/server.h @@ -83,6 +83,7 @@ class ValidationInstance : Logger::Loggable, } Runtime::Loader& runtime() override { return *runtime_loader_; } void shutdown() override; + bool isShutdown() override { return false; } void shutdownAdmin() override { NOT_IMPLEMENTED_GCOVR_EXCL_LINE; } Singleton::Manager& singletonManager() override { return *singleton_manager_; } OverloadManager& overloadManager() override { return *overload_manager_; } diff --git a/source/server/server.cc b/source/server/server.cc index f2baa4ef37df..dd166d336660 100644 --- a/source/server/server.cc +++ b/source/server/server.cc @@ -50,7 +50,7 @@ InstanceImpl::InstanceImpl(Options& options, Event::TimeSystem& time_system, ComponentFactory& component_factory, Runtime::RandomGeneratorPtr&& random_generator, ThreadLocal::Instance& tls) - : options_(options), time_system_(time_system), restarter_(restarter), + : shutdown_(false), options_(options), time_system_(time_system), restarter_(restarter), start_time_(time(nullptr)), original_start_time_(start_time_), stats_store_(store), thread_local_(tls), api_(new Api::Impl(options.fileFlushIntervalMsec())), secret_manager_(std::make_unique()), @@ -388,15 +388,15 @@ void InstanceImpl::loadServerFlags(const absl::optional& flags_path uint64_t InstanceImpl::numConnections() { return listener_manager_->numConnections(); } -RunHelper::RunHelper(Event::Dispatcher& dispatcher, Upstream::ClusterManager& cm, - HotRestart& hot_restart, AccessLog::AccessLogManager& access_log_manager, +RunHelper::RunHelper(Instance& instance, Event::Dispatcher& dispatcher, + Upstream::ClusterManager& cm, AccessLog::AccessLogManager& access_log_manager, InitManagerImpl& init_manager, OverloadManager& overload_manager, std::function workers_start_cb) { // Setup signals. - sigterm_ = dispatcher.listenForSignal(SIGTERM, [this, &hot_restart, &dispatcher]() { + sigterm_ = dispatcher.listenForSignal(SIGTERM, [&instance]() { ENVOY_LOG(warn, "caught SIGTERM"); - shutdown(dispatcher, hot_restart); + instance.shutdown(); }); sig_usr_1_ = dispatcher.listenForSignal(SIGUSR1, [&access_log_manager]() { @@ -413,8 +413,8 @@ RunHelper::RunHelper(Event::Dispatcher& dispatcher, Upstream::ClusterManager& cm // this can fire immediately if all clusters have already initialized. Also note that we need // to guard against shutdown at two different levels since SIGTERM can come in once the run loop // starts. - cm.setInitializedCb([this, &init_manager, &cm, workers_start_cb]() { - if (shutdown_) { + cm.setInitializedCb([&instance, &init_manager, &cm, workers_start_cb]() { + if (instance.isShutdown()) { return; } @@ -424,8 +424,11 @@ RunHelper::RunHelper(Event::Dispatcher& dispatcher, Upstream::ClusterManager& cm cm.adsMux().pause(Config::TypeUrl::get().RouteConfiguration); ENVOY_LOG(info, "all clusters initialized. initializing init manager"); - init_manager.initialize([this, workers_start_cb]() { - if (shutdown_) { + + // Note: the lamda below should not capture "this" since the RunHelper object may + // have been destructed by the time it gets executed. + init_manager.initialize([&instance, workers_start_cb]() { + if (instance.isShutdown()) { return; } @@ -440,16 +443,10 @@ RunHelper::RunHelper(Event::Dispatcher& dispatcher, Upstream::ClusterManager& cm overload_manager.start(); } -void RunHelper::shutdown(Event::Dispatcher& dispatcher, HotRestart& hot_restart) { - shutdown_ = true; - hot_restart.terminateParent(); - dispatcher.exit(); -} - void InstanceImpl::run() { // We need the RunHelper to be available to call from InstanceImpl::shutdown() below, so // we save it as a member variable. - run_helper_ = std::make_unique(*dispatcher_, clusterManager(), restarter_, + run_helper_ = std::make_unique(*this, *dispatcher_, clusterManager(), access_log_manager_, init_manager_, overloadManager(), [this]() -> void { startWorkers(); }); @@ -505,8 +502,9 @@ void InstanceImpl::terminate() { Runtime::Loader& InstanceImpl::runtime() { return *runtime_loader_; } void InstanceImpl::shutdown() { - ASSERT(run_helper_.get() != nullptr); - run_helper_->shutdown(*dispatcher_, restarter_); + shutdown_ = true; + restarter_.terminateParent(); + dispatcher_->exit(); } void InstanceImpl::shutdownAdmin() { diff --git a/source/server/server.h b/source/server/server.h index 13472b748928..373b7f05b647 100644 --- a/source/server/server.h +++ b/source/server/server.h @@ -113,19 +113,14 @@ class InstanceUtil : Logger::Loggable { */ class RunHelper : Logger::Loggable { public: - RunHelper(Event::Dispatcher& dispatcher, Upstream::ClusterManager& cm, HotRestart& hot_restart, + RunHelper(Instance& instance, Event::Dispatcher& dispatcher, Upstream::ClusterManager& cm, AccessLog::AccessLogManager& access_log_manager, InitManagerImpl& init_manager, OverloadManager& overload_manager, std::function workers_start_cb); - // Helper function to inititate a shutdown. This can be triggered either by catching SIGTERM - // or be called from ServerImpl::shutdown(). - void shutdown(Event::Dispatcher& dispatcher, HotRestart& hot_restart); - private: Event::SignalEventPtr sigterm_; Event::SignalEventPtr sig_usr_1_; Event::SignalEventPtr sig_hup_; - bool shutdown_{}; }; /** @@ -170,6 +165,7 @@ class InstanceImpl : Logger::Loggable, public Instance { } Runtime::Loader& runtime() override; void shutdown() override; + bool isShutdown() override final { return shutdown_; } void shutdownAdmin() override; Singleton::Manager& singletonManager() override { return *singleton_manager_; } bool healthCheckFailed() override; @@ -196,6 +192,7 @@ class InstanceImpl : Logger::Loggable, public Instance { void startWorkers(); void terminate(); + bool shutdown_; Options& options_; Event::TimeSystem& time_system_; HotRestart& restarter_; diff --git a/test/mocks/server/mocks.h b/test/mocks/server/mocks.h index dba2ace1723f..c33578e8d068 100644 --- a/test/mocks/server/mocks.h +++ b/test/mocks/server/mocks.h @@ -329,6 +329,7 @@ class MockInstance : public Instance { MOCK_METHOD0(rateLimitClient_, RateLimit::Client*()); MOCK_METHOD0(runtime, Runtime::Loader&()); MOCK_METHOD0(shutdown, void()); + MOCK_METHOD0(isShutdown, bool()); MOCK_METHOD0(shutdownAdmin, void()); MOCK_METHOD0(singletonManager, Singleton::Manager&()); MOCK_METHOD0(startTimeCurrentEpoch, time_t()); diff --git a/test/server/server_test.cc b/test/server/server_test.cc index ab1dc31ee664..90a41300af7f 100644 --- a/test/server/server_test.cc +++ b/test/server/server_test.cc @@ -16,12 +16,14 @@ #include "gtest/gtest.h" using testing::_; +using testing::Assign; using testing::HasSubstr; using testing::InSequence; using testing::Invoke; using testing::InvokeWithoutArgs; using testing::Property; using testing::Ref; +using testing::Return; using testing::SaveArg; using testing::StrictMock; @@ -53,7 +55,7 @@ TEST(ServerInstanceUtil, flushHelper) { class RunHelperTest : public testing::Test { public: - RunHelperTest() { + RunHelperTest() : shutdown_(false) { InSequence s; sigterm_ = new Event::MockSignalEvent(&dispatcher_); @@ -61,15 +63,16 @@ class RunHelperTest : public testing::Test { sighup_ = new Event::MockSignalEvent(&dispatcher_); EXPECT_CALL(cm_, setInitializedCb(_)).WillOnce(SaveArg<0>(&cm_init_callback_)); EXPECT_CALL(overload_manager_, start()); + ON_CALL(server_, shutdown()).WillByDefault(Assign(&shutdown_, true)); - helper_ = std::make_unique(dispatcher_, cm_, hot_restart_, access_log_manager_, - init_manager_, overload_manager_, - [this] { start_workers_.ready(); }); + helper_ = + std::make_unique(server_, dispatcher_, cm_, access_log_manager_, init_manager_, + overload_manager_, [this] { start_workers_.ready(); }); } + NiceMock server_; NiceMock dispatcher_; NiceMock cm_; - NiceMock hot_restart_; NiceMock access_log_manager_; NiceMock overload_manager_; InitManagerImpl init_manager_; @@ -79,6 +82,7 @@ class RunHelperTest : public testing::Test { Event::MockSignalEvent* sigterm_; Event::MockSignalEvent* sigusr1_; Event::MockSignalEvent* sighup_; + bool shutdown_ = false; }; TEST_F(RunHelperTest, Normal) { @@ -89,6 +93,7 @@ TEST_F(RunHelperTest, Normal) { TEST_F(RunHelperTest, ShutdownBeforeCmInitialize) { EXPECT_CALL(start_workers_, ready()).Times(0); sigterm_->callback_(); + EXPECT_CALL(server_, isShutdown()).WillOnce(Return(shutdown_)); cm_init_callback_(); } @@ -99,6 +104,7 @@ TEST_F(RunHelperTest, ShutdownBeforeInitManagerInit) { EXPECT_CALL(target, initialize(_)); cm_init_callback_(); sigterm_->callback_(); + EXPECT_CALL(server_, isShutdown()).WillOnce(Return(shutdown_)); target.callback_(); }