Skip to content

Commit

Permalink
Get rid of shutdown.cpp/shutdown.h, use SignalInterrupt directly
Browse files Browse the repository at this point in the history
This change is mostly a refectoring that removes some code and gets rid of an
unnecessary layer of indirection after bitcoin#27861

But it is not a pure refactoring since StartShutdown, AbortShutdown, and
WaitForShutdown functions used to abort on failure, and the replacement code
logs or returns errors instead.
  • Loading branch information
ryanofsky committed Jul 7, 2023
1 parent 5b3b5d0 commit 12ec1e5
Show file tree
Hide file tree
Showing 10 changed files with 47 additions and 121 deletions.
2 changes: 0 additions & 2 deletions src/Makefile.am
Original file line number Diff line number Diff line change
Expand Up @@ -270,7 +270,6 @@ BITCOIN_CORE_H = \
script/sign.h \
script/signingprovider.h \
script/solver.h \
shutdown.h \
signet.h \
streams.h \
support/allocators/pool.h \
Expand Down Expand Up @@ -458,7 +457,6 @@ libbitcoin_node_a_SOURCES = \
rpc/signmessage.cpp \
rpc/txoutproof.cpp \
script/sigcache.cpp \
shutdown.cpp \
signet.cpp \
timedata.cpp \
torcontrol.cpp \
Expand Down
6 changes: 1 addition & 5 deletions src/bitcoind.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -20,7 +20,6 @@
#include <node/context.h>
#include <node/interface_ui.h>
#include <noui.h>
#include <shutdown.h>
#include <util/check.h>
#include <util/exception.h>
#include <util/strencodings.h>
Expand Down Expand Up @@ -185,7 +184,6 @@ static bool AppInit(NodeContext& node)
}

node.kernel = std::make_unique<kernel::Context>();
node.shutdown = &node.kernel->interrupt; // TEMPORARY: will go away when kernel->interrupt member is removed
if (!AppInitSanityChecks(*node.kernel))
{
// InitError will have been called with detailed error, which ends up on console
Expand Down Expand Up @@ -273,9 +271,7 @@ MAIN_FUNCTION
if (ProcessInitCommands(args)) return EXIT_SUCCESS;

// Start application
if (AppInit(node)) {
WaitForShutdown();
} else {
if (!AppInit(node) || !Assert(node.shutdown)->wait()) {
node.exit_status = EXIT_FAILURE;
}
Interrupt(node);
Expand Down
55 changes: 37 additions & 18 deletions src/init.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -66,7 +66,6 @@
#include <rpc/util.h>
#include <scheduler.h>
#include <script/sigcache.h>
#include <shutdown.h>
#include <sync.h>
#include <timedata.h>
#include <torcontrol.h>
Expand Down Expand Up @@ -176,9 +175,15 @@ static fs::path GetPidFile(const ArgsManager& args)
}
}

static std::optional<util::SignalInterrupt> g_shutdown;

void InitContext(NodeContext& node)
{
assert(!g_shutdown);
g_shutdown.emplace();

node.args = &gArgs;
node.shutdown = &*g_shutdown;
}

//////////////////////////////////////////////////////////////////////////////
Expand All @@ -192,11 +197,9 @@ void InitContext(NodeContext& node)
// The network-processing threads are all part of a thread group
// created by AppInit() or the Qt main() function.
//
// A clean exit happens when StartShutdown() or the SIGTERM
// signal handler sets ShutdownRequested(), which makes main thread's
// WaitForShutdown() interrupts the thread group.
// And then, WaitForShutdown() makes all other on-going threads
// in the thread group join the main thread.
// A clean exit happens when the SignalInterrupt object is triggered, which
// makes main thread's SignalInterrupt::wait() call return, and join all other
// ongoing threads in the thread group to the main thread.
// Shutdown() is then called to clean up database connections, and stop other
// threads that should only be stopped after the main network-processing
// threads have exited.
Expand All @@ -206,6 +209,11 @@ void InitContext(NodeContext& node)
// shutdown thing.
//

bool ShutdownRequested(node::NodeContext& node)
{
return bool{*(Assert(node.shutdown))};
}

#if HAVE_SYSTEM
static void ShutdownNotify(const ArgsManager& args)
{
Expand Down Expand Up @@ -373,7 +381,9 @@ void Shutdown(NodeContext& node)
#ifndef WIN32
static void HandleSIGTERM(int)
{
StartShutdown();
// Return value is intentionally ignored because there is not a better way
// of handling this failure in a signal handler.
(void)(*Assert(g_shutdown))();
}

static void HandleSIGHUP(int)
Expand All @@ -383,7 +393,10 @@ static void HandleSIGHUP(int)
#else
static BOOL WINAPI consoleCtrlHandler(DWORD dwCtrlType)
{
StartShutdown();
if (!(*Assert(g_shutdown))()) {
LogPrintf("Error: failed to send shutdown signal on Ctrl-C\n");
return false;
}
Sleep(INFINITE);
return true;
}
Expand Down Expand Up @@ -1145,11 +1158,13 @@ bool AppInitMain(NodeContext& node, interfaces::BlockAndHeaderTipInfo* tip_info)
}, std::chrono::minutes{1});

// Check disk space every 5 minutes to avoid db corruption.
node.scheduler->scheduleEvery([&args]{
node.scheduler->scheduleEvery([&args, &node]{
constexpr uint64_t min_disk_space = 50 << 20; // 50 MB
if (!CheckDiskSpace(args.GetBlocksDirPath(), min_disk_space)) {
LogPrintf("Shutting down due to lack of disk space!\n");
StartShutdown();
if (!(*Assert(node.shutdown))()) {
LogPrintf("Error: failed to send shutdown signal after disk space check\n");
}
}
}, std::chrono::minutes{5});

Expand Down Expand Up @@ -1487,7 +1502,7 @@ bool AppInitMain(NodeContext& node, interfaces::BlockAndHeaderTipInfo* tip_info)
}
LogPrintf("* Using %.1f MiB for in-memory UTXO set (plus up to %.1f MiB of unused mempool space)\n", cache_sizes.coins * (1.0 / 1024 / 1024), mempool_opts.max_size_bytes * (1.0 / 1024 / 1024));

for (bool fLoaded = false; !fLoaded && !ShutdownRequested();) {
for (bool fLoaded = false; !fLoaded && !ShutdownRequested(node);) {
node.mempool = std::make_unique<CTxMemPool>(mempool_opts);

node.chainman = std::make_unique<ChainstateManager>(*Assert(node.shutdown), chainman_opts, blockman_opts);
Expand Down Expand Up @@ -1554,7 +1569,7 @@ bool AppInitMain(NodeContext& node, interfaces::BlockAndHeaderTipInfo* tip_info)
return InitError(error);
}

if (!fLoaded && !ShutdownRequested()) {
if (!fLoaded && !ShutdownRequested(node)) {
// first suggest a reindex
if (!options.reindex) {
bool fRet = uiInterface.ThreadSafeQuestion(
Expand All @@ -1563,7 +1578,9 @@ bool AppInitMain(NodeContext& node, interfaces::BlockAndHeaderTipInfo* tip_info)
"", CClientUIInterface::MSG_ERROR | CClientUIInterface::BTN_ABORT);
if (fRet) {
fReindex = true;
AbortShutdown();
if (!Assert(node.shutdown)->reset()) {
LogPrintf("Internal error: failed to reset shutdown signal.\n");
}
} else {
LogPrintf("Aborted block database rebuild. Exiting.\n");
return false;
Expand All @@ -1577,7 +1594,7 @@ bool AppInitMain(NodeContext& node, interfaces::BlockAndHeaderTipInfo* tip_info)
// As LoadBlockIndex can take several minutes, it's possible the user
// requested to kill the GUI during the last operation. If so, exit.
// As the program has not fully started yet, Shutdown() is possibly overkill.
if (ShutdownRequested()) {
if (ShutdownRequested(node)) {
LogPrintf("Shutdown requested. Exiting.\n");
return false;
}
Expand Down Expand Up @@ -1698,7 +1715,9 @@ bool AppInitMain(NodeContext& node, interfaces::BlockAndHeaderTipInfo* tip_info)
ImportBlocks(chainman, vImportFiles);
if (args.GetBoolArg("-stopafterblockimport", DEFAULT_STOPAFTERBLOCKIMPORT)) {
LogPrintf("Stopping after block import\n");
StartShutdown();
if (!(*Assert(node.shutdown))()) {
LogPrintf("Error: failed to send shutdown signal after finishing block import\n");
}
return;
}

Expand All @@ -1718,16 +1737,16 @@ bool AppInitMain(NodeContext& node, interfaces::BlockAndHeaderTipInfo* tip_info)
// Wait for genesis block to be processed
{
WAIT_LOCK(g_genesis_wait_mutex, lock);
// We previously could hang here if StartShutdown() is called prior to
// We previously could hang here if shutdown was requested prior to
// ImportBlocks getting started, so instead we just wait on a timer to
// check ShutdownRequested() regularly.
while (!fHaveGenesis && !ShutdownRequested()) {
while (!fHaveGenesis && !ShutdownRequested(node)) {
g_genesis_wait_cv.wait_for(lock, std::chrono::milliseconds(500));
}
block_notify_genesis_wait_connection.disconnect();
}

if (ShutdownRequested()) {
if (ShutdownRequested(node)) {
return false;
}

Expand Down
4 changes: 3 additions & 1 deletion src/init.h
Original file line number Diff line number Diff line change
Expand Up @@ -26,8 +26,10 @@ namespace node {
struct NodeContext;
} // namespace node

/** Initialize node context variables. */
/** Initialize node context shutdown and args variables. */
void InitContext(node::NodeContext& node);
/** Return whether node shutdown was requested. */
bool ShutdownRequested(node::NodeContext& node);

/** Interrupt threads */
void Interrupt(node::NodeContext& node);
Expand Down
6 changes: 0 additions & 6 deletions src/kernel/context.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -14,12 +14,8 @@


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();
Expand All @@ -29,8 +25,6 @@ Context::Context()
Context::~Context()
{
ECC_Stop();
assert(g_context);
g_context = nullptr;
}

} // namespace kernel
15 changes: 0 additions & 15 deletions src/kernel/context.h
Original file line number Diff line number Diff line change
Expand Up @@ -18,24 +18,9 @@ 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
11 changes: 6 additions & 5 deletions src/node/interfaces.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -39,13 +39,13 @@
#include <primitives/transaction.h>
#include <rpc/protocol.h>
#include <rpc/server.h>
#include <shutdown.h>
#include <support/allocators/secure.h>
#include <sync.h>
#include <txmempool.h>
#include <uint256.h>
#include <univalue.h>
#include <util/check.h>
#include <util/signalinterrupt.h>
#include <util/translation.h>
#include <validation.h>
#include <validationinterface.h>
Expand Down Expand Up @@ -99,7 +99,6 @@ class NodeImpl : public Node
if (!AppInitParameterInteraction(args())) return false;

m_context->kernel = std::make_unique<kernel::Context>();
m_context->shutdown = &m_context->kernel->interrupt; // TEMPORARY: will go away when kernel->interrupt member is removed
if (!AppInitSanityChecks(*m_context->kernel)) return false;

if (!AppInitLockDataDirectory()) return false;
Expand All @@ -121,14 +120,16 @@ class NodeImpl : public Node
}
void startShutdown() override
{
StartShutdown();
if (!(*Assert(Assert(m_context)->shutdown))()) {
LogPrintf("Error: failed to send shutdown signal\n");
}
// Stop RPC for clean shutdown if any of waitfor* commands is executed.
if (args().GetBoolArg("-server", false)) {
InterruptRPC();
StopRPC();
}
}
bool shutdownRequested() override { return ShutdownRequested(); }
bool shutdownRequested() override { return ShutdownRequested(*Assert(m_context)); };
bool isSettingIgnored(const std::string& name) override
{
bool ignored = false;
Expand Down Expand Up @@ -747,7 +748,7 @@ class ChainImpl : public Chain
{
return chainman().IsInitialBlockDownload();
}
bool shutdownRequested() override { return ShutdownRequested(); }
bool shutdownRequested() override { return ShutdownRequested(m_node); }
void initMessage(const std::string& message) override { ::uiInterface.InitMessage(message); }
void initWarning(const bilingual_str& message) override { InitWarning(message); }
void initError(const bilingual_str& message) override { InitError(message); }
Expand Down
43 changes: 0 additions & 43 deletions src/shutdown.cpp

This file was deleted.

25 changes: 0 additions & 25 deletions src/shutdown.h

This file was deleted.

1 change: 0 additions & 1 deletion src/test/util/setup_common.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -40,7 +40,6 @@
#include <rpc/server.h>
#include <scheduler.h>
#include <script/sigcache.h>
#include <shutdown.h>
#include <streams.h>
#include <test/util/net.h>
#include <test/util/random.h>
Expand Down

0 comments on commit 12ec1e5

Please sign in to comment.