Skip to content

mobile: removing guarddog and watchdog #30896

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
alyssawilk merged 1 commit into from
Nov 28, 2023
Merged

mobile: removing guarddog and watchdog #30896

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion .github/workflows/mobile-perf.yml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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'
Expand Down
4 changes: 2 additions & 2 deletions envoy/server/listener_manager.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down
4 changes: 2 additions & 2 deletions envoy/server/worker.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down
3 changes: 3 additions & 0 deletions mobile/library/common/engine_common.cc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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)
Expand Down
2 changes: 1 addition & 1 deletion ...e/library/common/extensions/listener_managers/api_listener_manager/api_listener_manager.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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 {}
Expand Down
2 changes: 1 addition & 1 deletion source/extensions/listener_managers/listener_manager/listener_manager_impl.cc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Expand Down
2 changes: 1 addition & 1 deletion source/extensions/listener_managers/listener_manager/listener_manager_impl.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Expand Down
2 changes: 1 addition & 1 deletion source/server/BUILD
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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",
Expand Down Expand Up @@ -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",
Expand Down
6 changes: 6 additions & 0 deletions source/server/instance_impl.cc
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -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 {
Expand All @@ -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
1 change: 1 addition & 0 deletions source/server/instance_impl.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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_;
Expand Down
20 changes: 11 additions & 9 deletions source/server/server.cc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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"
Expand Down Expand Up @@ -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() {
Expand Down Expand Up @@ -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;
}
Expand Down Expand Up @@ -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(),
Copy link
Copy Markdown
Contributor

@abeyad abeyad Nov 27, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Just for my own understanding, I get why on L823, the watch dog is optional on the listener since EM doesn't use traditional listeners. But I'm not clear on why we want to make the watch dog optional for the main thread?

Copy link
Copy Markdown
Contributor Author

@alyssawilk alyssawilk Nov 28, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

android already kills stuck apps - I don't think we need envoy internal handling for this

"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();
Expand Down
4 changes: 4 additions & 0 deletions source/server/server.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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;

Expand Down Expand Up @@ -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();

Expand Down
16 changes: 10 additions & 6 deletions source/server/worker_impl.cc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand All @@ -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); }
Expand All @@ -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
Expand Down
4 changes: 2 additions & 2 deletions source/server/worker_impl.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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);
Expand Down
2 changes: 1 addition & 1 deletion test/mocks/server/listener_manager.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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));
Expand Down
3 changes: 2 additions & 1 deletion test/mocks/server/worker.cc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Expand Down
2 changes: 1 addition & 1 deletion test/mocks/server/worker.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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,
Expand Down