mobile: removing guarddog and watchdog (envoyproxy#30896)

Signed-off-by: Alyssa Wilk <[email protected]>
fredyw · Nov 28, 2023 · 9c5feb6 · 9c5feb6
1 parent 24ee071
commit 9c5feb6
Show file tree

Hide file tree

Showing 17 changed files with 50 additions and 29 deletions.
diff --git a/.github/workflows/mobile-perf.yml b/.github/workflows/mobile-perf.yml
@@ -30,7 +30,7 @@ jobs:
     steps:
     - uses: actions/checkout@v4
     - name: Ensure files don't leak back into the main binary
-      run: rm source/extensions/listener_managers/listener_manager/listener_manager_impl.h source/server/overload_manager_impl.cc source/common/network/listen_socket_impl.h source/common/network/tcp_listener_impl.h
+      run: rm source/extensions/listener_managers/listener_manager/listener_manager_impl.h source/server/overload_manager_impl.cc source/common/network/listen_socket_impl.h source/common/network/tcp_listener_impl.h source/server/guarddog_impl.h source/server/watchdog_impl.h
     - name: Add safe directory
       run: git config --global --add safe.directory /__w/envoy/envoy
     - name: 'Build test binary'

diff --git a/envoy/server/listener_manager.h b/envoy/server/listener_manager.h
@@ -221,10 +221,10 @@ class ListenerManager {
 
   /**
    * Start all workers accepting new connections on all added listeners.
-   * @param guard_dog supplies the guard dog to use for thread watching.
+   * @param guard_dog supplies the optional guard dog to use for thread watching.
    * @param callback supplies the callback to complete server initialization.
    */
-  virtual void startWorkers(GuardDog& guard_dog, std::function<void()> callback) PURE;
+  virtual void startWorkers(OptRef<GuardDog> guard_dog, std::function<void()> callback) PURE;
 
   /**
    * Stop all listeners from accepting new connections without actually removing any of them. This

diff --git a/envoy/server/worker.h b/envoy/server/worker.h
@@ -45,10 +45,10 @@ class Worker {
 
   /**
    * Start the worker thread.
-   * @param guard_dog supplies the guard dog to use for thread watching.
+   * @param guard_dog supplies the optional guard dog to use for thread watching.
    * @param cb a callback to run when the worker thread starts running.
    */
-  virtual void start(GuardDog& guard_dog, const std::function<void()>& cb) PURE;
+  virtual void start(OptRef<GuardDog> guard_dog, const std::function<void()>& cb) PURE;
 
   /**
    * Initialize stats for this worker's dispatcher, if available. The worker will output

diff --git a/mobile/library/common/engine_common.cc b/mobile/library/common/engine_common.cc
@@ -62,6 +62,9 @@ class ServerLite : public Server::InstanceBase {
   std::unique_ptr<Envoy::Server::OverloadManager> createOverloadManager() override {
     return std::make_unique<Envoy::Server::NullOverloadManager>(threadLocal(), true);
   }
+  std::unique_ptr<Server::GuardDog> maybeCreateGuardDog(absl::string_view) override {
+    return nullptr;
+  }
 };
 
 EngineCommon::EngineCommon(std::unique_ptr<Envoy::OptionsImpl>&& options)

diff --git a/...e/library/common/extensions/listener_managers/api_listener_manager/api_listener_manager.h b/...e/library/common/extensions/listener_managers/api_listener_manager/api_listener_manager.h
@@ -29,7 +29,7 @@ class ApiListenerManagerImpl : public ListenerManager, Logger::Loggable<Logger::
   }
   uint64_t numConnections() const override { return 0; }
   bool removeListener(const std::string&) override { return true; }
-  void startWorkers(GuardDog&, std::function<void()> callback) override { callback(); }
+  void startWorkers(OptRef<GuardDog>, std::function<void()> callback) override { callback(); }
   void stopListeners(StopListenersType, const Network::ExtraShutdownListenerOptions&) override {}
   void stopWorkers() override {}
   void beginListenerUpdate() override {}

diff --git a/source/extensions/listener_managers/listener_manager/listener_manager_impl.cc b/source/extensions/listener_managers/listener_manager/listener_manager_impl.cc
@@ -919,7 +919,7 @@ bool ListenerManagerImpl::removeListenerInternal(const std::string& name,
   return true;
 }
 
-void ListenerManagerImpl::startWorkers(GuardDog& guard_dog, std::function<void()> callback) {
+void ListenerManagerImpl::startWorkers(OptRef<GuardDog> guard_dog, std::function<void()> callback) {
   ENVOY_LOG(info, "all dependencies initialized. starting workers");
   ASSERT(!workers_started_);
   workers_started_ = true;

diff --git a/source/extensions/listener_managers/listener_manager/listener_manager_impl.h b/source/extensions/listener_managers/listener_manager/listener_manager_impl.h
@@ -222,7 +222,7 @@ class ListenerManagerImpl : public ListenerManager, Logger::Loggable<Logger::Id:
   listeners(ListenerState state = ListenerState::ACTIVE) override;
   uint64_t numConnections() const override;
   bool removeListener(const std::string& listener_name) override;
-  void startWorkers(GuardDog& guard_dog, std::function<void()> callback) override;
+  void startWorkers(OptRef<GuardDog> guard_dog, std::function<void()> callback) override;
   void stopListeners(StopListenersType stop_listeners_type,
                      const Network::ExtraShutdownListenerOptions& options) override;
   void stopWorkers() override;

diff --git a/source/server/BUILD b/source/server/BUILD
@@ -395,7 +395,6 @@ envoy_cc_library(
     deps = [
         ":api_listener_lib",
         ":configuration_lib",
-        ":guarddog_lib",
         ":listener_hooks_lib",
         ":listener_manager_factory_lib",
         ":regex_engine_lib",
@@ -457,6 +456,7 @@ envoy_cc_library(
     srcs = ["instance_impl.cc"],
     hdrs = ["instance_impl.h"],
     deps = [
+        ":guarddog_lib",
         ":server_base_lib",
         "//source/common/memory:heap_shrinker_lib",
         "//source/server:overload_manager_lib",

diff --git a/source/server/instance_impl.cc b/source/server/instance_impl.cc
@@ -1,5 +1,6 @@
 #include "source/server/instance_impl.h"
 
+#include "source/server/guarddog_impl.h"
 #include "source/server/overload_manager_impl.h"
 
 namespace Envoy {
@@ -15,5 +16,10 @@ std::unique_ptr<OverloadManager> InstanceImpl::createOverloadManager() {
       messageValidationContext().staticValidationVisitor(), api(), options());
 }
 
+std::unique_ptr<Server::GuardDog> InstanceImpl::maybeCreateGuardDog(absl::string_view name) {
+  return std::make_unique<Server::GuardDogImpl>(*stats().rootScope(),
+                                                config().mainThreadWatchdogConfig(), api(), name);
+}
+
 } // namespace Server
 } // namespace Envoy
diff --git a/source/server/instance_impl.h b/source/server/instance_impl.h
@@ -14,6 +14,7 @@ class InstanceImpl : public InstanceBase {
 protected:
   void maybeCreateHeapShrinker() override;
   std::unique_ptr<OverloadManager> createOverloadManager() override;
+  std::unique_ptr<Server::GuardDog> maybeCreateGuardDog(absl::string_view name) override;
 
 private:
   std::unique_ptr<Memory::HeapShrinker> heap_shrinker_;

diff --git a/source/server/server.cc b/source/server/server.cc
@@ -51,7 +51,6 @@
 #include "source/common/upstream/cluster_manager_impl.h"
 #include "source/common/version/version.h"
 #include "source/server/configuration_impl.h"
-#include "source/server/guarddog_impl.h"
 #include "source/server/listener_hooks.h"
 #include "source/server/listener_manager_factory.h"
 #include "source/server/regex_engine.h"
@@ -778,10 +777,8 @@ void InstanceBase::initializeOrThrow(Network::Address::InstanceConstSharedPtr lo
 
   // GuardDog (deadlock detection) object and thread setup before workers are
   // started and before our own run() loop runs.
-  main_thread_guard_dog_ = std::make_unique<Server::GuardDogImpl>(
-      *stats_store_.rootScope(), config_.mainThreadWatchdogConfig(), *api_, "main_thread");
-  worker_guard_dog_ = std::make_unique<Server::GuardDogImpl>(
-      *stats_store_.rootScope(), config_.workerWatchdogConfig(), *api_, "workers");
+  main_thread_guard_dog_ = maybeCreateGuardDog("main_thread");
+  worker_guard_dog_ = maybeCreateGuardDog("workers");
 }
 
 void InstanceBase::onClusterManagerPrimaryInitializationComplete() {
@@ -823,7 +820,7 @@ void InstanceBase::onRuntimeReady() {
 
 void InstanceBase::startWorkers() {
   // The callback will be called after workers are started.
-  listener_manager_->startWorkers(*worker_guard_dog_, [this]() {
+  listener_manager_->startWorkers(makeOptRefFromPtr(worker_guard_dog_.get()), [this]() {
     if (isShutdown()) {
       return;
     }
@@ -950,12 +947,17 @@ void InstanceBase::run() {
 
   // Run the main dispatch loop waiting to exit.
   ENVOY_LOG(info, "starting main dispatch loop");
-  auto watchdog = main_thread_guard_dog_->createWatchDog(api_->threadFactory().currentThreadId(),
-                                                         "main_thread", *dispatcher_);
+  WatchDogSharedPtr watchdog;
+  if (main_thread_guard_dog_) {
+    watchdog = main_thread_guard_dog_->createWatchDog(api_->threadFactory().currentThreadId(),
+                                                      "main_thread", *dispatcher_);
+  }
   dispatcher_->post([this] { notifyCallbacksForStage(Stage::Startup); });
   dispatcher_->run(Event::Dispatcher::RunType::Block);
   ENVOY_LOG(info, "main dispatch loop exited");
-  main_thread_guard_dog_->stopWatching(watchdog);
+  if (main_thread_guard_dog_) {
+    main_thread_guard_dog_->stopWatching(watchdog);
+  }
   watchdog.reset();
 
   terminate();

diff --git a/source/server/server.h b/source/server/server.h
@@ -249,6 +249,7 @@ class InstanceBase : Logger::Loggable<Logger::Id::main>,
 
   virtual void maybeCreateHeapShrinker() PURE;
   virtual std::unique_ptr<OverloadManager> createOverloadManager() PURE;
+  virtual std::unique_ptr<Server::GuardDog> maybeCreateGuardDog(absl::string_view name) PURE;
 
   void run() override;
 
@@ -314,6 +315,9 @@ class InstanceBase : Logger::Loggable<Logger::Id::main>,
   ServerLifecycleNotifier::HandlePtr
   registerCallback(Stage stage, StageCallbackWithCompletion callback) override;
 
+protected:
+  const Configuration::MainImpl& config() { return config_; }
+
 private:
   Network::DnsResolverSharedPtr getOrCreateDnsResolver();
 

diff --git a/source/server/worker_impl.cc b/source/server/worker_impl.cc
@@ -98,7 +98,7 @@ void WorkerImpl::removeFilterChains(uint64_t listener_tag,
       });
 }
 
-void WorkerImpl::start(GuardDog& guard_dog, const std::function<void()>& cb) {
+void WorkerImpl::start(OptRef<GuardDog> guard_dog, const std::function<void()>& cb) {
   ASSERT(!thread_);
 
   // In posix, thread names are limited to 15 characters, so contrive to make
@@ -113,7 +113,7 @@ void WorkerImpl::start(GuardDog& guard_dog, const std::function<void()>& cb) {
   // architecture is centralized, resulting in clearer names.
   Thread::Options options{absl::StrCat("wrk:", dispatcher_->name())};
   thread_ = api_.threadFactory().createThread(
-      [this, &guard_dog, cb]() -> void { threadRoutine(guard_dog, cb); }, options);
+      [this, guard_dog, cb]() -> void { threadRoutine(guard_dog, cb); }, options);
 }
 
 void WorkerImpl::initializeStats(Stats::Scope& scope) { dispatcher_->initializeStats(scope); }
@@ -139,18 +139,22 @@ void WorkerImpl::stopListener(Network::ListenerConfig& listener,
   });
 }
 
-void WorkerImpl::threadRoutine(GuardDog& guard_dog, const std::function<void()>& cb) {
+void WorkerImpl::threadRoutine(OptRef<GuardDog> guard_dog, const std::function<void()>& cb) {
   ENVOY_LOG(debug, "worker entering dispatch loop");
   // The watch dog must be created after the dispatcher starts running and has post events flushed,
   // as this is when TLS stat scopes start working.
   dispatcher_->post([this, &guard_dog, cb]() {
     cb();
-    watch_dog_ = guard_dog.createWatchDog(api_.threadFactory().currentThreadId(),
-                                          dispatcher_->name(), *dispatcher_);
+    if (guard_dog.has_value()) {
+      watch_dog_ = guard_dog->createWatchDog(api_.threadFactory().currentThreadId(),
+                                             dispatcher_->name(), *dispatcher_);
+    }
   });
   dispatcher_->run(Event::Dispatcher::RunType::Block);
   ENVOY_LOG(debug, "worker exited dispatch loop");
-  guard_dog.stopWatching(watch_dog_);
+  if (guard_dog.has_value()) {
+    guard_dog->stopWatching(watch_dog_);
+  }
   dispatcher_->shutdown();
 
   // We must close all active connections before we actually exit the thread. This prevents any

diff --git a/source/server/worker_impl.h b/source/server/worker_impl.h
@@ -61,15 +61,15 @@ class WorkerImpl : public Worker, Logger::Loggable<Logger::Id::main> {
   void removeFilterChains(uint64_t listener_tag,
                           const std::list<const Network::FilterChain*>& filter_chains,
                           std::function<void()> completion) override;
-  void start(GuardDog& guard_dog, const std::function<void()>& cb) override;
+  void start(OptRef<GuardDog> guard_dog, const std::function<void()>& cb) override;
   void initializeStats(Stats::Scope& scope) override;
   void stop() override;
   void stopListener(Network::ListenerConfig& listener,
                     const Network::ExtraShutdownListenerOptions& options,
                     std::function<void()> completion) override;
 
 private:
-  void threadRoutine(GuardDog& guard_dog, const std::function<void()>& cb);
+  void threadRoutine(OptRef<GuardDog> guard_dog, const std::function<void()>& cb);
   void stopAcceptingConnectionsCb(OverloadActionState state);
   void rejectIncomingConnectionsCb(OverloadActionState state);
   void resetStreamsUsingExcessiveMemory(OverloadActionState state);

diff --git a/test/mocks/server/listener_manager.h b/test/mocks/server/listener_manager.h
@@ -21,7 +21,7 @@ class MockListenerManager : public ListenerManager {
               (ListenerState state));
   MOCK_METHOD(uint64_t, numConnections, (), (const));
   MOCK_METHOD(bool, removeListener, (const std::string& listener_name));
-  MOCK_METHOD(void, startWorkers, (GuardDog & guard_dog, std::function<void()> callback));
+  MOCK_METHOD(void, startWorkers, (OptRef<GuardDog> guard_dog, std::function<void()> callback));
   MOCK_METHOD(void, stopListeners,
               (StopListenersType listeners_type,
                const Network::ExtraShutdownListenerOptions& options));

diff --git a/test/mocks/server/worker.cc b/test/mocks/server/worker.cc
@@ -47,7 +47,8 @@ MockWorker::MockWorker() {
       }));
 
   ON_CALL(*this, start(_, _))
-      .WillByDefault(Invoke([](GuardDog&, const std::function<void()>& cb) -> void { cb(); }));
+      .WillByDefault(
+          Invoke([](OptRef<GuardDog>, const std::function<void()>& cb) -> void { cb(); }));
 }
 
 MockWorker::~MockWorker() = default;

diff --git a/test/mocks/server/worker.h b/test/mocks/server/worker.h
@@ -37,7 +37,7 @@ class MockWorker : public Worker {
   MOCK_METHOD(uint64_t, numConnections, (), (const));
   MOCK_METHOD(void, removeListener,
               (Network::ListenerConfig & listener, std::function<void()> completion));
-  MOCK_METHOD(void, start, (GuardDog & guard_dog, const std::function<void()>& cb));
+  MOCK_METHOD(void, start, (OptRef<GuardDog> guard_dog, const std::function<void()>& cb));
   MOCK_METHOD(void, initializeStats, (Stats::Scope & scope));
   MOCK_METHOD(void, stop, ());
   MOCK_METHOD(void, stopListener,