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

[Win32 Signals] Add term and ctrl-c signal handlers #13954

Merged
merged 35 commits into from
Dec 8, 2020
Merged
Show file tree
Hide file tree
Changes from 30 commits
Commits
Show all changes
35 commits
Select commit Hold shift + click to select a range
c9939d9
unix builds
Nov 7, 2020
ab8b2e2
fix format 1
Nov 7, 2020
2819000
format vol2
Nov 7, 2020
3774592
Win32 changes + tests
Nov 9, 2020
f55a385
add tests
Nov 10, 2020
f5ae7e0
fix format
Nov 10, 2020
c5f267f
fix pyformat
Nov 10, 2020
4159136
fix typo
Nov 10, 2020
709cbac
working through build errors
Nov 10, 2020
3158e46
fix with runfiles
Nov 11, 2020
2a16309
use the same if conditions everywhere
Nov 11, 2020
e9fde7f
fix format + deps
Nov 11, 2020
598b933
PR comments and robust testing
Nov 18, 2020
8cce5c9
format changes
Nov 18, 2020
44d4602
order includes
Nov 18, 2020
4d900be
fix include order vol 2
Nov 18, 2020
a191b0c
remove extra empty line
Nov 18, 2020
a201731
switch format to use absl duration
Nov 18, 2020
c484a75
fix linux CI
Nov 18, 2020
3d71e04
fix linux signal name typo
Nov 18, 2020
b050126
fix windows crash
Nov 19, 2020
c7186df
fix build deps
Nov 21, 2020
e6cc642
try fixing clang-tidy
Nov 21, 2020
a295a16
fix build break
Nov 21, 2020
e9bb5f2
fix build tools format
Nov 21, 2020
d92345b
Merge remote-tracking branch 'upstream/master' into processManagement
Nov 30, 2020
67aaa42
attempt to fix clang_tidy
Nov 30, 2020
6cc663b
fix typo
Nov 30, 2020
c378f5f
Change from map to array
Dec 2, 2020
7550099
Spelling
Dec 2, 2020
30bbdfa
fix pr comments by greg
Dec 5, 2020
a28a4f0
fix spelling mistake
Dec 5, 2020
85e19e3
Merge remote-tracking branch 'upstream/master' into processManagement
Dec 7, 2020
63e7ced
fix allocation + add panic
Dec 7, 2020
1f1e211
remove double signal entry
Dec 7, 2020
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
1 change: 1 addition & 0 deletions .bazelrc
Original file line number Diff line number Diff line change
Expand Up @@ -298,6 +298,7 @@ build:windows --action_env=TMPDIR
build:windows --define signal_trace=disabled
build:windows --define hot_restart=disabled
build:windows --define tcmalloc=disabled
build:windows --define wasm=disabled
davinci26 marked this conversation as resolved.
Show resolved Hide resolved
ggreenway marked this conversation as resolved.
Show resolved Hide resolved
build:windows --define manual_stamp=manual_stamp
build:windows --cxxopt="/std:c++17"

Expand Down
2 changes: 1 addition & 1 deletion ci/run_clang_tidy.sh
Original file line number Diff line number Diff line change
Expand Up @@ -31,7 +31,7 @@ echo "Generating compilation database..."
# Do not run clang-tidy against win32 impl
# TODO(scw00): We should run clang-tidy against win32 impl once we have clang-cl support for Windows
function exclude_win32_impl() {
grep -v source/common/filesystem/win32/ | grep -v source/common/common/win32 | grep -v source/exe/win32 | grep -v source/common/api/win32
grep -v source/common/filesystem/win32/ | grep -v source/common/common/win32 | grep -v source/exe/win32 | grep -v source/common/api/win32 | grep -v source/common/event/win32
}

# Do not run clang-tidy against macOS impl
Expand Down
1 change: 0 additions & 1 deletion ci/windows_ci_steps.sh
Original file line number Diff line number Diff line change
Expand Up @@ -47,7 +47,6 @@ BAZEL_BUILD_OPTIONS=(
-c opt
--show_task_finish
--verbose_failures
--define "wasm=disabled"
"--test_output=errors"
"${BAZEL_BUILD_EXTRA_OPTIONS[@]}"
"${BAZEL_EXTRA_TEST_OPTIONS[@]}")
Expand Down
5 changes: 5 additions & 0 deletions include/envoy/common/platform.h
Original file line number Diff line number Diff line change
Expand Up @@ -65,6 +65,7 @@ typedef uint32_t mode_t;

typedef SOCKET os_fd_t;
typedef HANDLE filesystem_os_id_t; // NOLINT(modernize-use-using)
typedef DWORD signal_t; // NOLINT(modernize-use-using)
davinci26 marked this conversation as resolved.
Show resolved Hide resolved

typedef unsigned int sa_family_t;

Expand Down Expand Up @@ -151,6 +152,9 @@ struct msghdr {
#define HANDLE_ERROR_PERM ERROR_ACCESS_DENIED
#define HANDLE_ERROR_INVALID ERROR_INVALID_HANDLE

#define ENVOY_WIN32_SIGNAL_COUNT 1
#define ENVOY_WIN32_SIGTERM 0

namespace Platform {
constexpr absl::string_view null_device_path{"NUL"};
}
Expand Down Expand Up @@ -215,6 +219,7 @@ constexpr absl::string_view null_device_path{"NUL"};

typedef int os_fd_t;
typedef int filesystem_os_id_t; // NOLINT(modernize-use-using)
typedef int signal_t; // NOLINT(modernize-use-using)

#define INVALID_HANDLE -1
#define INVALID_SOCKET -1
Expand Down
2 changes: 1 addition & 1 deletion include/envoy/event/dispatcher.h
Original file line number Diff line number Diff line change
Expand Up @@ -205,7 +205,7 @@ class Dispatcher {
* @param cb supplies the callback to invoke when the signal fires.
* @return SignalEventPtr a signal event that is owned by the caller.
*/
virtual SignalEventPtr listenForSignal(int signal_num, SignalCb cb) PURE;
virtual SignalEventPtr listenForSignal(signal_t signal_num, SignalCb cb) PURE;

/**
* Posts a functor to the dispatcher. This is safe cross thread. The functor runs in the context
Expand Down
51 changes: 47 additions & 4 deletions source/common/event/BUILD
Original file line number Diff line number Diff line change
@@ -1,6 +1,9 @@
load(
"//bazel:envoy_build_system.bzl",
"envoy_cc_library",
"envoy_cc_platform_dep",
"envoy_cc_posix_library",
"envoy_cc_win32_library",
"envoy_package",
)

Expand All @@ -13,15 +16,24 @@ envoy_cc_library(
srcs = [
"dispatcher_impl.cc",
"file_event_impl.cc",
"signal_impl.cc",
],
hdrs = [
"signal_impl.h",
],
hdrs = select({
"//bazel:windows_x86_64": [
"win32/signal_impl.h",
],
"//conditions:default": [
"posix/signal_impl.h",
],
}),
davinci26 marked this conversation as resolved.
Show resolved Hide resolved
strip_include_prefix = select({
"//bazel:windows_x86_64": "win32",
"//conditions:default": "posix",
}),
deps = [
":dispatcher_includes",
":libevent_scheduler_lib",
":real_time_system_lib",
":signal_lib",
"//include/envoy/common:scope_tracker_interface",
"//include/envoy/common:time_interface",
"//include/envoy/event:signal_interface",
Expand All @@ -39,6 +51,37 @@ envoy_cc_library(
}),
)

envoy_cc_library(
name = "signal_lib",
deps = envoy_cc_platform_dep("signal_impl_lib"),
)

envoy_cc_posix_library(
name = "signal_impl_lib",
srcs = ["posix/signal_impl.cc"],
hdrs = ["posix/signal_impl.h"],
strip_include_prefix = "posix",
antoniovicente marked this conversation as resolved.
Show resolved Hide resolved
deps = [
":dispatcher_includes",
"//include/envoy/event:signal_interface",
"//source/common/common:thread_lib",
],
)

envoy_cc_win32_library(
name = "signal_impl_lib",
srcs = ["win32/signal_impl.cc"],
hdrs = ["win32/signal_impl.h"],
strip_include_prefix = "win32",
deps = [
":dispatcher_includes",
"//include/envoy/event:signal_interface",
"//source/common/api:os_sys_calls_lib",
"//source/common/common:thread_lib",
"//source/common/network:default_socket_interface_lib",
],
)

envoy_cc_library(
name = "event_impl_base_lib",
srcs = ["event_impl_base.cc"],
Expand Down
2 changes: 1 addition & 1 deletion source/common/event/dispatcher_impl.cc
Original file line number Diff line number Diff line change
Expand Up @@ -218,7 +218,7 @@ void DispatcherImpl::deferredDelete(DeferredDeletablePtr&& to_delete) {

void DispatcherImpl::exit() { base_scheduler_.loopExit(); }

SignalEventPtr DispatcherImpl::listenForSignal(int signal_num, SignalCb cb) {
SignalEventPtr DispatcherImpl::listenForSignal(signal_t signal_num, SignalCb cb) {
ASSERT(isThreadSafe());
return SignalEventPtr{new SignalEventImpl(*this, signal_num, cb)};
}
Expand Down
2 changes: 1 addition & 1 deletion source/common/event/dispatcher_impl.h
Original file line number Diff line number Diff line change
Expand Up @@ -71,7 +71,7 @@ class DispatcherImpl : Logger::Loggable<Logger::Id::main>,
Event::SchedulableCallbackPtr createSchedulableCallback(std::function<void()> cb) override;
void deferredDelete(DeferredDeletablePtr&& to_delete) override;
void exit() override;
SignalEventPtr listenForSignal(int signal_num, SignalCb cb) override;
SignalEventPtr listenForSignal(signal_t signal_num, SignalCb cb) override;
void post(std::function<void()> callback) override;
void run(RunType type) override;
Buffer::WatermarkFactory& getWatermarkFactory() override { return *buffer_factory_; }
Expand Down
Original file line number Diff line number Diff line change
@@ -1,13 +1,12 @@
#include "common/event/signal_impl.h"

#include "common/event/dispatcher_impl.h"
#include "common/event/signal_impl.h"

#include "event2/event.h"

namespace Envoy {
namespace Event {

SignalEventImpl::SignalEventImpl(DispatcherImpl& dispatcher, int signal_num, SignalCb cb)
SignalEventImpl::SignalEventImpl(DispatcherImpl& dispatcher, signal_t signal_num, SignalCb cb)
: cb_(cb) {
evsignal_assign(
&raw_event_, &dispatcher.base(), signal_num,
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -13,11 +13,10 @@ namespace Event {
*/
class SignalEventImpl : public SignalEvent, ImplBase {
public:
SignalEventImpl(DispatcherImpl& dispatcher, int signal_num, SignalCb cb);
SignalEventImpl(DispatcherImpl& dispatcher, signal_t signal_num, SignalCb cb);

private:
SignalCb cb_;
};

} // namespace Event
} // namespace Envoy
45 changes: 45 additions & 0 deletions source/common/event/win32/signal_impl.cc
Original file line number Diff line number Diff line change
@@ -0,0 +1,45 @@
#include "common/api/os_sys_calls_impl.h"
#include "common/event/dispatcher_impl.h"
#include "common/event/signal_impl.h"

#include "event2/event.h"

namespace Envoy {
namespace Event {

SignalEventImpl::SignalEventImpl(DispatcherImpl& dispatcher, signal_t signal_num, SignalCb cb)
: cb_(cb) {

if (signal_num > eventBridgeHandlersSingleton::get().size()) {
ENVOY_BUG(false, "Attempting to create SignalEventImpl with a signal id that exceeds the "
davinci26 marked this conversation as resolved.
Show resolved Hide resolved
"number of supported signals.");
return;
}

if (eventBridgeHandlersSingleton::get()[signal_num]) {
return;
}
os_fd_t socks[2];
Api::SysCallIntResult result =
Api::OsSysCallsSingleton::get().socketpair(AF_INET, SOCK_STREAM, IPPROTO_TCP, socks);
ASSERT(result.rc_ == 0);

read_handle_ = std::make_unique<Network::IoSocketHandleImpl>(socks[0], false, AF_INET);
result = read_handle_->setBlocking(false);
ASSERT(result.rc_ == 0);
auto write_handle = std::make_shared<Network::IoSocketHandleImpl>(socks[1], false, AF_INET);
result = write_handle->setBlocking(false);
ASSERT(result.rc_ == 0);
davinci26 marked this conversation as resolved.
Show resolved Hide resolved

read_handle_->initializeFileEvent(
dispatcher,
[this](uint32_t events) -> void {
ASSERT(events == Event::FileReadyType::Read);
cb_();
},
Event::FileTriggerType::Level, Event::FileReadyType::Read);
eventBridgeHandlersSingleton::get()[signal_num] = write_handle;
}

} // namespace Event
} // namespace Envoy
35 changes: 35 additions & 0 deletions source/common/event/win32/signal_impl.h
Original file line number Diff line number Diff line change
@@ -0,0 +1,35 @@
#pragma once

#include "envoy/event/signal.h"
#include "envoy/network/io_handle.h"

#include "common/event/dispatcher_impl.h"
#include "common/event/event_impl_base.h"
#include "common/network/io_socket_handle_impl.h"
#include "common/singleton/threadsafe_singleton.h"

#include "absl/container/flat_hash_map.h"

namespace Envoy {
namespace Event {

/**
* libevent implementation of Event::SignalEvent.
*/
class SignalEventImpl : public SignalEvent {
public:
SignalEventImpl(DispatcherImpl& dispatcher, signal_t signal_num, SignalCb cb);

private:
SignalCb cb_;
Network::IoHandlePtr read_handle_;
};

// Windows ConsoleControlHandler does not allow for a context. As a result the thread
// spawned to handle the console events communicates with the main program with this socketpair.
// Here we have a map from signal types to IoHandle. When we write to this handle we trigger an
// event that notifies Envoy to act on the signal.
using eventBridgeHandlersSingleton =
ThreadSafeSingleton<std::array<std::shared_ptr<Network::IoHandle>, ENVOY_WIN32_SIGNAL_COUNT>>;
} // namespace Event
} // namespace Envoy
2 changes: 2 additions & 0 deletions source/exe/BUILD
Original file line number Diff line number Diff line change
Expand Up @@ -168,8 +168,10 @@ envoy_cc_win32_library(
srcs = ["win32/platform_impl.cc"],
deps = [
":platform_header_lib",
"//source/common/buffer:buffer_lib",
"//source/common/common:assert_lib",
"//source/common/common:thread_lib",
"//source/common/event:signal_lib",
"//source/common/filesystem:filesystem_lib",
],
)
Expand Down
43 changes: 43 additions & 0 deletions source/exe/win32/platform_impl.cc
Original file line number Diff line number Diff line change
@@ -1,17 +1,60 @@
#include <chrono>
#include <thread>

#include "common/buffer/buffer_impl.h"
#include "common/common/assert.h"
#include "common/common/thread_impl.h"
#include "common/event/signal_impl.h"
#include "common/filesystem/filesystem_impl.h"

#include "exe/platform_impl.h"

namespace Envoy {

static volatile bool shutdown_pending = false;
davinci26 marked this conversation as resolved.
Show resolved Hide resolved

BOOL WINAPI CtrlHandler(DWORD fdwCtrlType) {
if (shutdown_pending) {
return 0;
}
shutdown_pending = true;

auto handler = Event::eventBridgeHandlersSingleton::get()[ENVOY_WIN32_SIGTERM];
if (!handler) {
return 0;
}

// This code is executed as part of a thread running under a Windows
davinci26 marked this conversation as resolved.
Show resolved Hide resolved
// context. For that reason we want to avoid allocating memory or
// taking locks. This is why we use do not want to just write to a socket
// to wake up the signal handler.
Buffer::OwnedImpl buffer;
constexpr absl::string_view data{"a"};
buffer.add(data);
davinci26 marked this conversation as resolved.
Show resolved Hide resolved
auto result = handler->write(buffer);
RELEASE_ASSERT(result.rc_ == 1,
fmt::format("failed to write 1 byte: {}", result.err_->getErrorDetails()));

if (fdwCtrlType == CTRL_LOGOFF_EVENT || fdwCtrlType == CTRL_SHUTDOWN_EVENT) {
// These events terminate the process immediately so we want to give a couple of seconds
// to the dispatcher to shutdown the server.
constexpr size_t delay = 3;
absl::SleepFor(absl::Seconds(delay));
}
return 1;
}

PlatformImpl::PlatformImpl()
: thread_factory_(std::make_unique<Thread::ThreadFactoryImplWin32>()),
file_system_(std::make_unique<Filesystem::InstanceImplWin32>()) {
WSADATA wsa_data;
const WORD version_requested = MAKEWORD(2, 2);
RELEASE_ASSERT(WSAStartup(version_requested, &wsa_data) == 0, "WSAStartup failed with error");

if (!SetConsoleCtrlHandler(CtrlHandler, 1)) {
// The Control Handler is executing in a different thread.
ENVOY_LOG_MISC(warn, "Could not set Windows Control Handlers. Continuing without them.");
}
}

PlatformImpl::~PlatformImpl() { ::WSACleanup(); }
Expand Down
9 changes: 6 additions & 3 deletions source/server/server.cc
Original file line number Diff line number Diff line change
Expand Up @@ -644,9 +644,12 @@ RunHelper::RunHelper(Instance& instance, const Options& options, Event::Dispatch
}) {
// Setup signals.
if (options.signalHandlingEnabled()) {
// TODO(Pivotal): Figure out solution to graceful shutdown on Windows. None of these signals exist
// on Windows.
#ifndef WIN32
#ifdef WIN32
sigterm_ = dispatcher.listenForSignal(ENVOY_WIN32_SIGTERM, [&instance]() {
davinci26 marked this conversation as resolved.
Show resolved Hide resolved
ENVOY_LOG(warn, "caught ENVOY_WIN32_SIGTERM");
instance.shutdown();
});
#else
sigterm_ = dispatcher.listenForSignal(SIGTERM, [&instance]() {
ENVOY_LOG(warn, "caught SIGTERM");
instance.shutdown();
Expand Down
21 changes: 21 additions & 0 deletions test/exe/BUILD
Original file line number Diff line number Diff line change
Expand Up @@ -81,3 +81,24 @@ envoy_cc_test(
"//test/test_common:utility_lib",
],
)

# Due to the limitiations of how Windows signals work
# this test cannot be executed through bazel and it must
# be executed as a standalone executable.
# e.g.: `./bazel-bin/test/exe/win32_outofproc_main_test.exe`
envoy_cc_test(
davinci26 marked this conversation as resolved.
Show resolved Hide resolved
name = "win32_outofproc_main_test",
srcs = ["win32_outofproc_main_test.cc"],
data = [
"//source/exe:envoy-static",
"//test/config/integration:google_com_proxy_port_0",
],
tags = ["manual"],
deps = [
"//source/common/api:api_lib",
"//source/exe:main_common_lib",
"//test/mocks/runtime:runtime_mocks",
"//test/test_common:contention_lib",
"//test/test_common:environment_lib",
],
)
Loading