Fix envoy crash during early shutdown

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.
    */

source/server/config_validation/server.h

@@ -83,6 +83,7 @@ class ValidationInstance : Logger::Loggable<Logger::Id::main>,
   }
   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_; }

source/server/server.cc

@@ -49,7 +49,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())),
       dispatcher_(api_->allocateDispatcher(time_system)),
@@ -387,15 +387,15 @@ void InstanceImpl::loadServerFlags(const absl::optional<std::string>& 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<void()> 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]() {
@@ -412,8 +412,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;
     }
 
@@ -423,8 +423,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;
       }
 
@@ -439,16 +442,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<RunHelper>(*dispatcher_, clusterManager(), restarter_,
+  run_helper_ = std::make_unique<RunHelper>(*this, *dispatcher_, clusterManager(),
                                             access_log_manager_, init_manager_, overloadManager(),
                                             [this]() -> void { startWorkers(); });
 
@@ -504,8 +501,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() {

source/server/server.h

@@ -113,19 +113,14 @@ class InstanceUtil : Logger::Loggable<Logger::Id::main> {
  */
 class RunHelper : Logger::Loggable<Logger::Id::main> {
 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<void()> 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<Logger::Id::main>, public Instance {
   }
   Runtime::Loader& runtime() override;
   void shutdown() override;
+  bool isShutdown() override { return shutdown_; }
   void shutdownAdmin() override;
   Singleton::Manager& singletonManager() override { return *singleton_manager_; }
   bool healthCheckFailed() override;
@@ -196,6 +192,7 @@ class InstanceImpl : Logger::Loggable<Logger::Id::main>, public Instance {
   void startWorkers();
   void terminate();
 
+  bool shutdown_;
   Options& options_;
   Event::TimeSystem& time_system_;
   HotRestart& restarter_;

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());

test/server/server_test.cc

@@ -14,12 +14,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;
 
@@ -51,22 +53,24 @@ TEST(ServerInstanceUtil, flushHelper) {
 
 class RunHelperTest : public testing::Test {
 public:
-  RunHelperTest() {
+  RunHelperTest() : shutdown_(false) {
     InSequence s;
 
     sigterm_ = new Event::MockSignalEvent(&dispatcher_);
     sigusr1_ = new Event::MockSignalEvent(&dispatcher_);
     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_.reset(new RunHelper(dispatcher_, cm_, hot_restart_, access_log_manager_, init_manager_,
-                                overload_manager_, [this] { start_workers_.ready(); }));
+    helper_ =
+        std::make_unique<RunHelper>(server_, dispatcher_, cm_, access_log_manager_, init_manager_,
+                                    overload_manager_, [this] { start_workers_.ready(); });
   }
 
+  NiceMock<MockInstance> server_;
   NiceMock<Event::MockDispatcher> dispatcher_;
   NiceMock<Upstream::MockClusterManager> cm_;
-  NiceMock<MockHotRestart> hot_restart_;
   NiceMock<AccessLog::MockAccessLogManager> access_log_manager_;
   NiceMock<MockOverloadManager> overload_manager_;
   InitManagerImpl init_manager_;
@@ -76,6 +80,7 @@ class RunHelperTest : public testing::Test {
   Event::MockSignalEvent* sigterm_;
   Event::MockSignalEvent* sigusr1_;
   Event::MockSignalEvent* sighup_;
+  bool shutdown_ = false;
 };
 
 TEST_F(RunHelperTest, Normal) {
@@ -86,6 +91,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_();
 }
 
@@ -96,6 +102,7 @@ TEST_F(RunHelperTest, ShutdownBeforeInitManagerInit) {
   EXPECT_CALL(target, initialize(_));
   cm_init_callback_();
   sigterm_->callback_();
+  EXPECT_CALL(server_, isShutdown()).WillOnce(Return(shutdown_));
   target.callback_();
 }