From cd03513dc2fcccaa142e9632a28b38efd0056436 Mon Sep 17 00:00:00 2001 From: "Wladimir J. van der Laan" Date: Tue, 8 Dec 2020 21:49:06 +0100 Subject: [PATCH] init: Signal-safe instant shutdown Replace the 200ms polling loop with a faster and more efficient waiting operation. This was tried a few times before, but given up every time because solutions use a condition variable which is not safe for use in signals as they need to be reentrant. On UNIX-ish OSes, use a safe way: a pipe. When shutdown is requested write a dummy byte to the pipe. Waiting for shutdown is a matter of a blocking read from the pipe. On Windows, there are no signals so using a condition variable is safe. --- src/bitcoind.cpp | 17 ++------- src/init.cpp | 3 ++ src/shutdown.cpp | 89 ++++++++++++++++++++++++++++++++++++++++++++++++ src/shutdown.h | 17 +++++++++ 4 files changed, 112 insertions(+), 14 deletions(-) diff --git a/src/bitcoind.cpp b/src/bitcoind.cpp index 4c89db54cb..b7bcb534ef 100644 --- a/src/bitcoind.cpp +++ b/src/bitcoind.cpp @@ -28,15 +28,6 @@ const std::function G_TRANSLATION_FUN = nullptr; UrlDecodeFn* const URL_DECODE = urlDecode; -static void WaitForShutdown(NodeContext& node) -{ - while (!ShutdownRequested()) - { - UninterruptibleSleep(std::chrono::milliseconds{200}); - } - Interrupt(node); -} - static bool AppInit(int argc, char* argv[]) { NodeContext node; @@ -147,12 +138,10 @@ static bool AppInit(int argc, char* argv[]) PrintExceptionContinue(nullptr, "AppInit()"); } - if (!fRet) - { - Interrupt(node); - } else { - WaitForShutdown(node); + if (fRet) { + WaitForShutdown(); } + Interrupt(node); Shutdown(node); return fRet; diff --git a/src/init.cpp b/src/init.cpp index 371399de9e..a9d8e7e40c 100644 --- a/src/init.cpp +++ b/src/init.cpp @@ -917,6 +917,9 @@ bool AppInitBasicSetup(const ArgsManager& args) // Enable heap terminate-on-corruption HeapSetInformation(nullptr, HeapEnableTerminationOnCorruption, nullptr, 0); #endif + if (!InitShutdownState()) { + return InitError(Untranslated("Initializing wait-for-shutdown state failed.")); + } if (!SetupNetworking()) { return InitError(Untranslated("Initializing networking failed.")); diff --git a/src/shutdown.cpp b/src/shutdown.cpp index dec497d8ec..a3321a6106 100644 --- a/src/shutdown.cpp +++ b/src/shutdown.cpp @@ -5,19 +5,108 @@ #include +#include + +#include #include +#ifdef WIN32 +#include +#else +#include +#include +#include +#endif static std::atomic fRequestShutdown(false); +#ifdef WIN32 +/** On windows it is possible to simply use a condition variable. */ +std::mutex g_shutdown_mutex; +std::condition_variable g_shutdown_cv; +#else +/** On UNIX-like operating systems use the self-pipe trick. + * Index 0 will be the read end of the pipe, index 1 the write end. + */ +static int g_shutdown_pipe[2] = {-1, -1}; +#endif + +bool InitShutdownState() +{ +#ifndef WIN32 +#if HAVE_O_CLOEXEC + // If we can, make sure that the file descriptors are closed on exec() + // to prevent interference. + if (pipe2(g_shutdown_pipe, O_CLOEXEC) != 0) { + return false; + } +#else + if (pipe(g_shutdown_pipe) != 0) { + return false; + } +#endif +#endif + return true; +} void StartShutdown() { +#ifdef WIN32 + std::unique_lock lk(g_shutdown_mutex); fRequestShutdown = true; + g_shutdown_cv.notify_one(); +#else + // This must be reentrant and safe for calling in a signal handler, so using a condition variable is not safe. + // Make sure that the token is only written once even if multiple threads call this concurrently or in + // case of a reentrant signal. + if (!fRequestShutdown.exchange(true)) { + // Write an arbitrary byte to the write end of the shutdown pipe. + const char token = 'x'; + while (true) { + int result = write(g_shutdown_pipe[1], &token, 1); + if (result < 0) { + // Failure. It's possible that the write was interrupted by another signal. + // Other errors are unexpected here. + assert(errno == EINTR); + } else { + assert(result == 1); + break; + } + } + } +#endif } + void AbortShutdown() { + if (fRequestShutdown) { + // Cancel existing shutdown by waiting for it, this will reset condition flags and remove + // the shutdown token from the pipe. + WaitForShutdown(); + } fRequestShutdown = false; } + bool ShutdownRequested() { return fRequestShutdown; } + +void WaitForShutdown() +{ +#ifdef WIN32 + std::unique_lock lk(g_shutdown_mutex); + g_shutdown_cv.wait(lk, [] { return fRequestShutdown.load(); }); +#else + char token; + while (true) { + int result = read(g_shutdown_pipe[0], &token, 1); + if (result < 0) { + // Failure. Check if the read was interrupted by a signal. + // Other errors are unexpected here. + assert(errno == EINTR); + } else { + assert(result == 1); + break; + } + } +#endif +} diff --git a/src/shutdown.h b/src/shutdown.h index 3ed851c789..23f84179e9 100644 --- a/src/shutdown.h +++ b/src/shutdown.h @@ -6,8 +6,25 @@ #ifndef BITCOIN_SHUTDOWN_H #define BITCOIN_SHUTDOWN_H +/** Initialize shutdown state. This must be called before using either StartShutdown(), + * AbortShutdown() or WaitForShutdown(). Calling ShutdownRequested() is always safe. + */ +bool InitShutdownState(); + +/** Request shutdown of the application. */ void StartShutdown(); + +/** Clear shutdown flag. Only use this during init (before calling WaitForShutdown in any + * thread), or in the unit tests. Calling it in other circumstances will cause a race condition. + */ void AbortShutdown(); + +/** Returns true if a shutdown is requested, false otherwise. */ bool ShutdownRequested(); +/** Wait for StartShutdown to be called in any thread. This can only be used + * from a single thread. + */ +void WaitForShutdown(); + #endif