Skip to content
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.

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

Extract SignalHandler class for re-use #282

Merged
merged 10 commits into from
Jun 18, 2020
38 changes: 2 additions & 36 deletions source/client/service_main.cc
Original file line number Diff line number Diff line change
Expand Up @@ -16,11 +16,6 @@
namespace Nighthawk {
namespace Client {

namespace {
std::function<void(int)> 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
Expand Down Expand Up @@ -87,44 +82,15 @@ void ServiceMain::start() {
}
channel_ = grpc::CreateChannel(listener_bound_address_, grpc::InsecureChannelCredentials());
stub_ = std::make_unique<nighthawk::client::NighthawkService::Stub>(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<SignalHandler>([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");
}
void ServiceMain::shutdown() { ENVOY_LOG(info, "Nighthawk grpc service exits"); }

} // namespace Client
} // namespace Nighthawk
20 changes: 3 additions & 17 deletions source/client/service_main.h
Original file line number Diff line number Diff line change
Expand Up @@ -12,6 +12,8 @@

#include "api/client/service.pb.h"

#include "common/signal_handler.h"

#include "client/service_impl.h"

#include "tclap/CmdLine.h"
Expand All @@ -37,17 +39,6 @@ class ServiceMain : public Envoy::Logger::Loggable<Envoy::Logger::Id::main> {
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_;
std::unique_ptr<grpc::Service> service_;
std::unique_ptr<grpc::Server> server_;
Expand All @@ -56,12 +47,7 @@ class ServiceMain : public Envoy::Logger::Loggable<Envoy::Logger::Id::main> {
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<int> pipe_fds_;
std::thread shutdown_thread_;
SignalHandlerPtr signal_handler_;
};

} // namespace Client
Expand Down
2 changes: 2 additions & 0 deletions source/common/BUILD
Original file line number Diff line number Diff line change
Expand Up @@ -67,6 +67,7 @@ envoy_cc_library(
"phase_impl.cc",
"rate_limiter_impl.cc",
"sequencer_impl.cc",
"signal_handler.cc",
"statistic_impl.cc",
"termination_predicate_impl.cc",
"uri_impl.cc",
Expand All @@ -81,6 +82,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",
Expand Down
50 changes: 50 additions & 0 deletions source/common/signal_handler.cc
Original file line number Diff line number Diff line change
@@ -0,0 +1,50 @@
#include "common/signal_handler.h"

#include <csignal>

#include "external/envoy/source/common/common/assert.h"
#include "external/envoy/source/common/common/macros.h"

namespace Nighthawk {

namespace {
std::function<void(int)> signal_handler_delegate;
void signal_handler(int signal) { signal_handler_delegate(signal); }
} // namespace

SignalHandler::SignalHandler(const std::function<void()>& 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
76 changes: 76 additions & 0 deletions source/common/signal_handler.h
Original file line number Diff line number Diff line change
@@ -0,0 +1,76 @@
#pragma once

#include <functional>
#include <memory>
#include <thread>
#include <vector>

#include "external/envoy/source/common/common/logger.h"

namespace Nighthawk {

/**
* Callback definition for providing a delegate that should be executed after a signal
* is observed.
*/
using SignalCallback = std::function<void()>;
Copy link
Collaborator

Choose a reason for hiding this comment

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

Since this is part of the header - can we document it?


/**
* 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<Envoy::Logger::Id::main> {
Copy link
Collaborator

Choose a reason for hiding this comment

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

Let's also add documentation for the class including an example of usage.

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);
Copy link
Collaborator

Choose a reason for hiding this comment

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

Can we add documentation for the constructor?


// Not copyable or movable.
SignalHandler(SignalHandler const&) = delete;
Copy link
Collaborator

Choose a reason for hiding this comment

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

Can we add a single comment above these two to indicate the intention, something like:

// Not copyable or movable.

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<int> pipe_fds_;
};

using SignalHandlerPtr = std::unique_ptr<SignalHandler>;

} // namespace Nighthawk