From b973b75a2527a0c89e4eaafc04887211a27bfe31 Mon Sep 17 00:00:00 2001 From: Otto van der Schaaf Date: Wed, 15 Jan 2020 16:21:15 +0100 Subject: [PATCH 1/6] Extract SignalHandler In preparation of sharing functionality for signal handling, extract what we have right now into SignalHandler. Status: draft For fixing #280 Signed-off-by: Otto van der Schaaf --- source/client/service_main.cc | 38 ++----------------------- source/client/service_main.h | 19 ++----------- source/common/BUILD | 2 ++ source/common/signal_handler.cc | 50 +++++++++++++++++++++++++++++++++ source/common/signal_handler.h | 44 +++++++++++++++++++++++++++++ 5 files changed, 101 insertions(+), 52 deletions(-) create mode 100644 source/common/signal_handler.cc create mode 100644 source/common/signal_handler.h diff --git a/source/client/service_main.cc b/source/client/service_main.cc index ee27b36b7..378e0ba59 100644 --- a/source/client/service_main.cc +++ b/source/client/service_main.cc @@ -5,22 +5,16 @@ #include "nighthawk/common/exception.h" +#include "client/service_impl.h" #include "common/utility.h" #include "common/version_info.h" -#include "client/service_impl.h" - #include "absl/strings/strip.h" #include "tclap/CmdLine.h" namespace Nighthawk { namespace Client { -namespace { -std::function signal_handler_delegate; -void signal_handler(int signal) { signal_handler_delegate(signal); } -} // namespace - ServiceMain::ServiceMain(int argc, const char** argv) { const char* descr = "L7 (HTTP/HTTPS/HTTP2) performance characterization tool."; TCLAP::CmdLine cmd(descr, ' ', VersionInfo::version()); // NOLINT @@ -76,43 +70,17 @@ void ServiceMain::start() { } channel_ = grpc::CreateChannel(listener_bound_address_, grpc::InsecureChannelCredentials()); stub_ = std::make_unique(channel_); - pipe_fds_.resize(2); - // The shutdown thread will be notified of by our signal handler and take it from there. - RELEASE_ASSERT(pipe(pipe_fds_.data()) == 0, "pipe failed"); - - shutdown_thread_ = std::thread([this]() { - int tmp; - RELEASE_ASSERT(read(pipe_fds_[0], &tmp, sizeof(int)) >= 0, "read failed"); - RELEASE_ASSERT(close(pipe_fds_[0]) == 0, "read side close failed"); - RELEASE_ASSERT(close(pipe_fds_[1]) == 0, "write side close failed"); - pipe_fds_.clear(); - server_->Shutdown(); - }); + signal_handler_ = std::make_unique([this]() { server_->Shutdown(); }); } void ServiceMain::wait() { - signal_handler_delegate = [this](int) { onSignal(); }; - signal(SIGTERM, signal_handler); - signal(SIGINT, signal_handler); server_->Wait(); shutdown(); } -void ServiceMain::onSignal() { initiateShutdown(); } - -void ServiceMain::initiateShutdown() { - if (pipe_fds_.size() == 2) { - const int tmp = 0; - RELEASE_ASSERT(write(pipe_fds_[1], &tmp, sizeof(int)) == sizeof(int), "write failed"); - } -} - void ServiceMain::shutdown() { - initiateShutdown(); - if (shutdown_thread_.joinable()) { - shutdown_thread_.join(); - } ENVOY_LOG(info, "Nighthawk grpc service exits"); + std::cerr << "3" << std::endl; } } // namespace Client diff --git a/source/client/service_main.h b/source/client/service_main.h index e4a814069..f08504e86 100644 --- a/source/client/service_main.h +++ b/source/client/service_main.h @@ -13,6 +13,7 @@ #include "api/client/service.pb.h" #include "client/service_impl.h" +#include "common/signal_handler.h" #include "tclap/CmdLine.h" @@ -37,17 +38,6 @@ class ServiceMain : public Envoy::Logger::Loggable { static std::string appendDefaultPortIfNeeded(absl::string_view host_and_maybe_port); private: - /** - * Notifies the thread responsible for shutting down the server that it is time to do so, if - * needed. Safe to use in signal handling, and non-blocking. - */ - void initiateShutdown(); - - /** - * Fires on signal reception. - */ - void onSignal(); - grpc::ServerBuilder builder_; ServiceImpl service_; std::unique_ptr server_; @@ -56,12 +46,7 @@ class ServiceMain : public Envoy::Logger::Loggable { int listener_port_{-1}; std::string listener_bound_address_; std::string listener_output_path_; - // Signal handling needs to be lean so we can't directly initiate shutdown while handling a - // signal. Therefore we write a bite to a this pipe to propagate signal reception. Subsequently, - // the read side will handle the actual shut down of the gRPC service without having to worry - // about signal-safety. - std::vector pipe_fds_; - std::thread shutdown_thread_; + SignalHandlerPtr signal_handler_; }; } // namespace Client diff --git a/source/common/BUILD b/source/common/BUILD index 8fad24e18..60a356bb1 100644 --- a/source/common/BUILD +++ b/source/common/BUILD @@ -43,6 +43,7 @@ envoy_cc_library( srcs = [ "rate_limiter_impl.cc", "sequencer_impl.cc", + "signal_handler.cc", "statistic_impl.cc", "termination_predicate_impl.cc", "uri_impl.cc", @@ -55,6 +56,7 @@ envoy_cc_library( "platform_util_impl.h", "rate_limiter_impl.h", "sequencer_impl.h", + "signal_handler.h", "statistic_impl.h", "termination_predicate_impl.h", "uri_impl.h", diff --git a/source/common/signal_handler.cc b/source/common/signal_handler.cc new file mode 100644 index 000000000..b7361d2a3 --- /dev/null +++ b/source/common/signal_handler.cc @@ -0,0 +1,50 @@ +#include "common/signal_handler.h" + +#include + +#include "external/envoy/source/common/common/assert.h" +#include "external/envoy/source/common/common/macros.h" + +namespace Nighthawk { + +namespace { +std::function signal_handler_delegate; +void signal_handler(int signal) { signal_handler_delegate(signal); } +} // namespace + +SignalHandler::SignalHandler(std::function signal_callback) { + pipe_fds_.resize(2); + // The shutdown thread will be notified of by our signal handler and take it from there. + RELEASE_ASSERT(pipe(pipe_fds_.data()) == 0, "pipe failed"); + + shutdown_thread_ = std::thread([this, signal_callback]() { + int tmp; + RELEASE_ASSERT(read(pipe_fds_[0], &tmp, sizeof(int)) >= 0, "read failed"); + RELEASE_ASSERT(close(pipe_fds_[0]) == 0, "read side close failed"); + RELEASE_ASSERT(close(pipe_fds_[1]) == 0, "write side close failed"); + pipe_fds_.clear(); + signal_callback(); + }); + + signal_handler_delegate = [this](int) { onSignal(); }; + signal(SIGTERM, signal_handler); + signal(SIGINT, signal_handler); +} + +SignalHandler::~SignalHandler() { + initiateShutdown(); + if (shutdown_thread_.joinable()) { + shutdown_thread_.join(); + } +} + +void SignalHandler::initiateShutdown() { + if (pipe_fds_.size() == 2) { + const int tmp = 0; + RELEASE_ASSERT(write(pipe_fds_[1], &tmp, sizeof(int)) == sizeof(int), "write failed"); + } +} + +void SignalHandler::onSignal() { initiateShutdown(); } + +} // namespace Nighthawk \ No newline at end of file diff --git a/source/common/signal_handler.h b/source/common/signal_handler.h new file mode 100644 index 000000000..988e444b3 --- /dev/null +++ b/source/common/signal_handler.h @@ -0,0 +1,44 @@ +#pragma once + +#include +#include +#include +#include + +#include "external/envoy/source/common/common/logger.h" + +namespace Nighthawk { + +using SignalCallback = std::function; + +class SignalHandler final : public Envoy::Logger::Loggable { +public: + SignalHandler(SignalCallback signal_callback); + SignalHandler(SignalHandler const&) = delete; + void operator=(SignalHandler const&) = delete; + ~SignalHandler(); + +private: + /** + * Fires on signal reception. + */ + void onSignal(); + + /** + * Notifies the thread responsible for shutting down the server that it is time to do so, if + * needed. Safe to use in signal handling, and non-blocking. + */ + void initiateShutdown(); + + std::thread shutdown_thread_; + + // Signal handling needs to be lean so we can't directly initiate shutdown while handling a + // signal. Therefore we write a bite to a this pipe to propagate signal reception. Subsequently, + // the read side will handle the actual shut down of the gRPC service without having to worry + // about signal-safety. + std::vector pipe_fds_; +}; + +using SignalHandlerPtr = std::unique_ptr; + +} // namespace Nighthawk \ No newline at end of file From fb5c4347cf4f4294d3cf4d71ddf563efb80df7be Mon Sep 17 00:00:00 2001 From: Otto van der Schaaf Date: Wed, 15 Jan 2020 16:38:12 +0100 Subject: [PATCH 2/6] Remove accidentally left in comment Signed-off-by: Otto van der Schaaf --- source/client/service_main.cc | 5 +---- 1 file changed, 1 insertion(+), 4 deletions(-) diff --git a/source/client/service_main.cc b/source/client/service_main.cc index 378e0ba59..a60155c75 100644 --- a/source/client/service_main.cc +++ b/source/client/service_main.cc @@ -78,10 +78,7 @@ void ServiceMain::wait() { shutdown(); } -void ServiceMain::shutdown() { - ENVOY_LOG(info, "Nighthawk grpc service exits"); - std::cerr << "3" << std::endl; -} +void ServiceMain::shutdown() { ENVOY_LOG(info, "Nighthawk grpc service exits"); } } // namespace Client } // namespace Nighthawk From 1bd1d31a2053b3017f8f8b05265e0d9b7f35d5b9 Mon Sep 17 00:00:00 2001 From: Otto van der Schaaf Date: Wed, 15 Jan 2020 16:56:49 +0100 Subject: [PATCH 3/6] Fix format Signed-off-by: Otto van der Schaaf --- source/client/service_main.cc | 3 ++- source/client/service_main.h | 3 ++- 2 files changed, 4 insertions(+), 2 deletions(-) diff --git a/source/client/service_main.cc b/source/client/service_main.cc index a60155c75..a357d09c2 100644 --- a/source/client/service_main.cc +++ b/source/client/service_main.cc @@ -5,10 +5,11 @@ #include "nighthawk/common/exception.h" -#include "client/service_impl.h" #include "common/utility.h" #include "common/version_info.h" +#include "client/service_impl.h" + #include "absl/strings/strip.h" #include "tclap/CmdLine.h" diff --git a/source/client/service_main.h b/source/client/service_main.h index f08504e86..97ae86109 100644 --- a/source/client/service_main.h +++ b/source/client/service_main.h @@ -12,9 +12,10 @@ #include "api/client/service.pb.h" -#include "client/service_impl.h" #include "common/signal_handler.h" +#include "client/service_impl.h" + #include "tclap/CmdLine.h" namespace Nighthawk { From ce5ead6a60328526234090a76df8fe23a69449ee Mon Sep 17 00:00:00 2001 From: Otto van der Schaaf Date: Wed, 15 Jan 2020 20:50:40 +0100 Subject: [PATCH 4/6] Amend according to clang-tidy's complaints Signed-off-by: Otto van der Schaaf --- source/common/signal_handler.cc | 4 ++-- source/common/signal_handler.h | 2 +- 2 files changed, 3 insertions(+), 3 deletions(-) diff --git a/source/common/signal_handler.cc b/source/common/signal_handler.cc index b7361d2a3..8c8a0c4a2 100644 --- a/source/common/signal_handler.cc +++ b/source/common/signal_handler.cc @@ -1,6 +1,6 @@ #include "common/signal_handler.h" -#include +#include #include "external/envoy/source/common/common/assert.h" #include "external/envoy/source/common/common/macros.h" @@ -12,7 +12,7 @@ std::function signal_handler_delegate; void signal_handler(int signal) { signal_handler_delegate(signal); } } // namespace -SignalHandler::SignalHandler(std::function signal_callback) { +SignalHandler::SignalHandler(const std::function signal_callback) { pipe_fds_.resize(2); // The shutdown thread will be notified of by our signal handler and take it from there. RELEASE_ASSERT(pipe(pipe_fds_.data()) == 0, "pipe failed"); diff --git a/source/common/signal_handler.h b/source/common/signal_handler.h index 988e444b3..ae989566b 100644 --- a/source/common/signal_handler.h +++ b/source/common/signal_handler.h @@ -13,7 +13,7 @@ using SignalCallback = std::function; class SignalHandler final : public Envoy::Logger::Loggable { public: - SignalHandler(SignalCallback signal_callback); + SignalHandler(const SignalCallback signal_callback); SignalHandler(SignalHandler const&) = delete; void operator=(SignalHandler const&) = delete; ~SignalHandler(); From 3f991199ee82bd0c5c87dfa6e3e59c9723443b2e Mon Sep 17 00:00:00 2001 From: Otto van der Schaaf Date: Wed, 15 Jan 2020 21:18:02 +0100 Subject: [PATCH 5/6] Pass const ref Signed-off-by: Otto van der Schaaf --- source/common/signal_handler.cc | 2 +- source/common/signal_handler.h | 2 +- 2 files changed, 2 insertions(+), 2 deletions(-) diff --git a/source/common/signal_handler.cc b/source/common/signal_handler.cc index 8c8a0c4a2..aa9316e66 100644 --- a/source/common/signal_handler.cc +++ b/source/common/signal_handler.cc @@ -12,7 +12,7 @@ std::function signal_handler_delegate; void signal_handler(int signal) { signal_handler_delegate(signal); } } // namespace -SignalHandler::SignalHandler(const std::function signal_callback) { +SignalHandler::SignalHandler(const std::function& signal_callback) { pipe_fds_.resize(2); // The shutdown thread will be notified of by our signal handler and take it from there. RELEASE_ASSERT(pipe(pipe_fds_.data()) == 0, "pipe failed"); diff --git a/source/common/signal_handler.h b/source/common/signal_handler.h index ae989566b..38bce2531 100644 --- a/source/common/signal_handler.h +++ b/source/common/signal_handler.h @@ -13,7 +13,7 @@ using SignalCallback = std::function; class SignalHandler final : public Envoy::Logger::Loggable { public: - SignalHandler(const SignalCallback signal_callback); + SignalHandler(const SignalCallback& signal_callback); SignalHandler(SignalHandler const&) = delete; void operator=(SignalHandler const&) = delete; ~SignalHandler(); From e665347410d7dc6e74212cc6fc6286a8d076b418 Mon Sep 17 00:00:00 2001 From: Otto van der Schaaf Date: Thu, 18 Jun 2020 10:19:39 +0200 Subject: [PATCH 6/6] Review feedback Signed-off-by: Otto van der Schaaf --- source/common/signal_handler.h | 32 ++++++++++++++++++++++++++++++++ 1 file changed, 32 insertions(+) diff --git a/source/common/signal_handler.h b/source/common/signal_handler.h index 38bce2531..3ff36374f 100644 --- a/source/common/signal_handler.h +++ b/source/common/signal_handler.h @@ -9,11 +9,43 @@ namespace Nighthawk { +/** + * Callback definition for providing a delegate that should be executed after a signal + * is observed. + */ using SignalCallback = std::function; +/** + * Utility class for handling TERM and INT signals. Allows wiring up a callback that + * should be invoked upon signal reception. This callback implementation does not have to be + * signal safe, as a different thread is used to fire it. + * NOTE: Only the first observed signal will result in the callback being invoked. + * WARNING: only a single instance should be active at any given time in a process, and + * the responsibility for not breaking this rule is not enforced at this time. + * + * Example usage: + * + * Process p; + * { + * // Signals will be handled while in this scope. + * // The provided callback will call cancel(), gracefully terminating + * // execution. + * auto s = SignalHandler([&p]() { log("cancelling!"); p->cancel(); }); + * p->executeInfinitelyOrUntilCancelled(); + * } + * + */ class SignalHandler final : public Envoy::Logger::Loggable { public: + /** + * Constructs a new SignalHandler instance. + * WARNING: Only a single instance is allowed to be active process-wide at any given time. + * @param signal_callback will be invoked after the first signal gets caught. Does not need to be + * signal-safe. + */ SignalHandler(const SignalCallback& signal_callback); + + // Not copyable or movable. SignalHandler(SignalHandler const&) = delete; void operator=(SignalHandler const&) = delete; ~SignalHandler();