From e2d680a32d757de0ef8eb836047a0daa1d82e3c4 Mon Sep 17 00:00:00 2001 From: TheCharlatan Date: Thu, 1 Jun 2023 16:53:33 -0400 Subject: util: Add SignalInterrupt class and use in shutdown.cpp This change helps generalize shutdown code so an interrupt can be provided to libbitcoinkernel callers. This may also be useful to eventually de-globalize all of the shutdown code. Co-authored-by: Russell Yanofsky Co-authored-by: TheCharlatan --- src/Makefile.am | 3 ++ src/init.cpp | 4 +-- src/kernel/context.cpp | 5 +++ src/kernel/context.h | 14 +++++++++ src/qt/test/apptests.cpp | 1 - src/shutdown.cpp | 69 +++++++++-------------------------------- src/shutdown.h | 2 +- src/util/signalinterrupt.cpp | 74 ++++++++++++++++++++++++++++++++++++++++++++ src/util/signalinterrupt.h | 52 +++++++++++++++++++++++++++++++ src/util/threadinterrupt.h | 16 +++++++--- 10 files changed, 175 insertions(+), 65 deletions(-) create mode 100644 src/util/signalinterrupt.cpp create mode 100644 src/util/signalinterrupt.h (limited to 'src') diff --git a/src/Makefile.am b/src/Makefile.am index 85c3eaf08d..fe0b621340 100644 --- a/src/Makefile.am +++ b/src/Makefile.am @@ -310,6 +310,7 @@ BITCOIN_CORE_H = \ util/readwritefile.h \ util/result.h \ util/serfloat.h \ + util/signalinterrupt.h \ util/sock.h \ util/spanparsing.h \ util/string.h \ @@ -733,6 +734,7 @@ libbitcoin_util_a_SOURCES = \ util/moneystr.cpp \ util/rbf.cpp \ util/readwritefile.cpp \ + util/signalinterrupt.cpp \ util/thread.cpp \ util/threadinterrupt.cpp \ util/threadnames.cpp \ @@ -972,6 +974,7 @@ libbitcoinkernel_la_SOURCES = \ util/moneystr.cpp \ util/rbf.cpp \ util/serfloat.cpp \ + util/signalinterrupt.cpp \ util/strencodings.cpp \ util/string.cpp \ util/syserror.cpp \ diff --git a/src/init.cpp b/src/init.cpp index c38352ee38..e9421606ef 100644 --- a/src/init.cpp +++ b/src/init.cpp @@ -812,9 +812,7 @@ bool AppInitBasicSetup(const ArgsManager& args, std::atomic& exit_status) // Enable heap terminate-on-corruption HeapSetInformation(nullptr, HeapEnableTerminationOnCorruption, nullptr, 0); #endif - if (!InitShutdownState(exit_status)) { - return InitError(Untranslated("Initializing wait-for-shutdown state failed.")); - } + InitShutdownState(exit_status); if (!SetupNetworking()) { return InitError(Untranslated("Initializing networking failed.")); diff --git a/src/kernel/context.cpp b/src/kernel/context.cpp index 1205da869e..3f4a423531 100644 --- a/src/kernel/context.cpp +++ b/src/kernel/context.cpp @@ -14,9 +14,12 @@ namespace kernel { +Context* g_context; Context::Context() { + assert(!g_context); + g_context = this; std::string sha256_algo = SHA256AutoDetect(); LogPrintf("Using the '%s' SHA256 implementation\n", sha256_algo); RandomInit(); @@ -26,6 +29,8 @@ Context::Context() Context::~Context() { ECC_Stop(); + assert(g_context); + g_context = nullptr; } } // namespace kernel diff --git a/src/kernel/context.h b/src/kernel/context.h index f11b7b54f0..e74e0a6f08 100644 --- a/src/kernel/context.h +++ b/src/kernel/context.h @@ -5,6 +5,8 @@ #ifndef BITCOIN_KERNEL_CONTEXT_H #define BITCOIN_KERNEL_CONTEXT_H +#include + #include namespace kernel { @@ -16,12 +18,24 @@ namespace kernel { //! State stored directly in this struct should be simple. More complex state //! should be stored to std::unique_ptr members pointing to opaque types. struct Context { + //! Interrupt object that can be used to stop long-running kernel operations. + util::SignalInterrupt interrupt; + //! Declare default constructor and destructor that are not inline, so code //! instantiating the kernel::Context struct doesn't need to #include class //! definitions for all the unique_ptr members. Context(); ~Context(); }; + +//! Global pointer to kernel::Context for legacy code. New code should avoid +//! using this, and require state it needs to be passed to it directly. +//! +//! Having this pointer is useful because it allows state be moved out of global +//! variables into the kernel::Context struct before all global references to +//! that state are removed. This allows the global references to be removed +//! incrementally, instead of all at once. +extern Context* g_context; } // namespace kernel #endif // BITCOIN_KERNEL_CONTEXT_H diff --git a/src/qt/test/apptests.cpp b/src/qt/test/apptests.cpp index e918e84184..fb8029cb65 100644 --- a/src/qt/test/apptests.cpp +++ b/src/qt/test/apptests.cpp @@ -86,7 +86,6 @@ void AppTests::appTests() // Reset global state to avoid interfering with later tests. LogInstance().DisconnectTestLogger(); - AbortShutdown(); } //! Entry point for BitcoinGUI tests. diff --git a/src/shutdown.cpp b/src/shutdown.cpp index d70017d734..185cad5baf 100644 --- a/src/shutdown.cpp +++ b/src/shutdown.cpp @@ -9,17 +9,15 @@ #include #endif +#include #include #include #include -#include +#include #include -#include #include -#ifdef WIN32 -#include -#endif +#include static std::atomic* g_exit_status{nullptr}; @@ -36,76 +34,37 @@ bool AbortNode(const std::string& strMessage, bilingual_str user_message) return false; } -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. - */ -static TokenPipeEnd g_shutdown_r; -static TokenPipeEnd g_shutdown_w; -#endif - -bool InitShutdownState(std::atomic& exit_status) +void InitShutdownState(std::atomic& exit_status) { g_exit_status = &exit_status; -#ifndef WIN32 - std::optional pipe = TokenPipe::Make(); - if (!pipe) return false; - g_shutdown_r = pipe->TakeReadEnd(); - g_shutdown_w = pipe->TakeWriteEnd(); -#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. - int res = g_shutdown_w.TokenWrite('x'); - if (res != 0) { - LogPrintf("Sending shutdown token failed\n"); - assert(0); - } + try { + Assert(kernel::g_context)->interrupt(); + } catch (const std::system_error&) { + LogPrintf("Sending shutdown token failed\n"); + assert(0); } -#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; + Assert(kernel::g_context)->interrupt.reset(); } bool ShutdownRequested() { - return fRequestShutdown; + return bool{Assert(kernel::g_context)->interrupt}; } void WaitForShutdown() { -#ifdef WIN32 - std::unique_lock lk(g_shutdown_mutex); - g_shutdown_cv.wait(lk, [] { return fRequestShutdown.load(); }); -#else - int res = g_shutdown_r.TokenRead(); - if (res != 'x') { + try { + Assert(kernel::g_context)->interrupt.wait(); + } catch (const std::system_error&) { LogPrintf("Reading shutdown token failed\n"); assert(0); } -#endif } diff --git a/src/shutdown.h b/src/shutdown.h index c119bee96f..0e51575c5a 100644 --- a/src/shutdown.h +++ b/src/shutdown.h @@ -16,7 +16,7 @@ bool AbortNode(const std::string& strMessage, bilingual_str user_message = bilin /** Initialize shutdown state. This must be called before using either StartShutdown(), * AbortShutdown() or WaitForShutdown(). Calling ShutdownRequested() is always safe. */ -bool InitShutdownState(std::atomic& exit_status); +void InitShutdownState(std::atomic& exit_status); /** Request shutdown of the application. */ void StartShutdown(); diff --git a/src/util/signalinterrupt.cpp b/src/util/signalinterrupt.cpp new file mode 100644 index 0000000000..c551ba8044 --- /dev/null +++ b/src/util/signalinterrupt.cpp @@ -0,0 +1,74 @@ +// Copyright (c) 2022 The Bitcoin Core developers +// Distributed under the MIT software license, see the accompanying +// file COPYING or http://www.opensource.org/licenses/mit-license.php. + +#include + +#ifdef WIN32 +#include +#else +#include +#endif + +#include +#include + +namespace util { + +SignalInterrupt::SignalInterrupt() : m_flag{false} +{ +#ifndef WIN32 + std::optional pipe = TokenPipe::Make(); + if (!pipe) throw std::ios_base::failure("Could not create TokenPipe"); + m_pipe_r = pipe->TakeReadEnd(); + m_pipe_w = pipe->TakeWriteEnd(); +#endif +} + +SignalInterrupt::operator bool() const +{ + return m_flag; +} + +void SignalInterrupt::reset() +{ + // Cancel existing interrupt by waiting for it, this will reset condition flags and remove + // the token from the pipe. + if (*this) wait(); + m_flag = false; +} + +void SignalInterrupt::operator()() +{ +#ifdef WIN32 + std::unique_lock lk(m_mutex); + m_flag = true; + m_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 (!m_flag.exchange(true)) { + // Write an arbitrary byte to the write end of the pipe. + int res = m_pipe_w.TokenWrite('x'); + if (res != 0) { + throw std::ios_base::failure("Could not write interrupt token"); + } + } +#endif +} + +void SignalInterrupt::wait() +{ +#ifdef WIN32 + std::unique_lock lk(m_mutex); + m_cv.wait(lk, [this] { return m_flag.load(); }); +#else + int res = m_pipe_r.TokenRead(); + if (res != 'x') { + throw std::ios_base::failure("Did not read expected interrupt token"); + } +#endif +} + +} // namespace util diff --git a/src/util/signalinterrupt.h b/src/util/signalinterrupt.h new file mode 100644 index 0000000000..ca02feda91 --- /dev/null +++ b/src/util/signalinterrupt.h @@ -0,0 +1,52 @@ +// Copyright (c) 2023 The Bitcoin Core developers +// Distributed under the MIT software license, see the accompanying +// file COPYING or http://www.opensource.org/licenses/mit-license.php. + +#ifndef BITCOIN_UTIL_SIGNALINTERRUPT_H +#define BITCOIN_UTIL_SIGNALINTERRUPT_H + +#ifdef WIN32 +#include +#include +#else +#include +#endif + +#include +#include + +namespace util { +/** + * Helper class that manages an interrupt flag, and allows a thread or + * signal to interrupt another thread. + * + * This class is safe to be used in a signal handler. If sending an interrupt + * from a signal handler is not necessary, the more lightweight \ref + * CThreadInterrupt class can be used instead. + */ + +class SignalInterrupt +{ +public: + SignalInterrupt(); + explicit operator bool() const; + void operator()(); + void reset(); + void wait(); + +private: + std::atomic m_flag; + +#ifndef WIN32 + // On UNIX-like operating systems use the self-pipe trick. + TokenPipeEnd m_pipe_r; + TokenPipeEnd m_pipe_w; +#else + // On windows use a condition variable, since we don't have any signals there + std::mutex m_mutex; + std::condition_variable m_cv; +#endif +}; +} // namespace util + +#endif // BITCOIN_UTIL_SIGNALINTERRUPT_H diff --git a/src/util/threadinterrupt.h b/src/util/threadinterrupt.h index ccc053f576..0b79b38276 100644 --- a/src/util/threadinterrupt.h +++ b/src/util/threadinterrupt.h @@ -12,11 +12,17 @@ #include #include -/* - A helper class for interruptible sleeps. Calling operator() will interrupt - any current sleep, and after that point operator bool() will return true - until reset. -*/ +/** + * A helper class for interruptible sleeps. Calling operator() will interrupt + * any current sleep, and after that point operator bool() will return true + * until reset. + * + * This class should not be used in a signal handler. It uses thread + * synchronization primitives that are not safe to use with signals. If sending + * an interrupt from a signal handler is necessary, the \ref SignalInterrupt + * class can be used instead. + */ + class CThreadInterrupt { public: -- cgit v1.2.3