-
Notifications
You must be signed in to change notification settings - Fork 38.6k
Get rid of shutdown.cpp/shutdown.h, use SignalInterrupt directly #28051
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
Changes from all commits
263b23f
f0c73c1
f4a8bd6
73133c3
42e5829
ba93966
1d92d89
6824eec
feeb7b8
213542b
6db04be
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -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> | ||
|
|
@@ -192,6 +191,16 @@ static void RemovePidFile(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; | ||
| } | ||
|
|
||
| ////////////////////////////////////////////////////////////////////////////// | ||
| // | ||
|
|
@@ -204,11 +213,9 @@ static void RemovePidFile(const ArgsManager& args) | |
| // 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 the 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. | ||
|
|
@@ -218,6 +225,11 @@ static void RemovePidFile(const ArgsManager& args) | |
| // shutdown thing. | ||
| // | ||
|
|
||
| bool ShutdownRequested(node::NodeContext& node) | ||
| { | ||
| return bool{*Assert(node.shutdown)}; | ||
| } | ||
|
|
||
| #if HAVE_SYSTEM | ||
| static void ShutdownNotify(const ArgsManager& args) | ||
| { | ||
|
|
@@ -382,7 +394,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) | ||
|
|
@@ -392,7 +406,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; | ||
| } | ||
ryanofsky marked this conversation as resolved.
Show resolved
Hide resolved
|
||
| Sleep(INFINITE); | ||
| return true; | ||
| } | ||
|
|
@@ -690,8 +707,9 @@ static bool AppInitServers(NodeContext& node) | |
| const ArgsManager& args = *Assert(node.args); | ||
| RPCServer::OnStarted(&OnRPCStarted); | ||
| RPCServer::OnStopped(&OnRPCStopped); | ||
| if (!InitHTTPServer()) | ||
| if (!InitHTTPServer(*Assert(node.shutdown))) { | ||
| return false; | ||
| } | ||
| StartRPC(); | ||
| node.rpc_interruption_point = RpcInterruptionPoint; | ||
| if (!StartHTTPRPC(&node)) | ||
|
|
@@ -1140,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"); | ||
| } | ||
|
Comment on lines
+1165
to
+1167
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This interface of dealing with shutdown failures feels a bit awkward to me. It's verbose and quite difficult to parse. These shutdown failures aren't really related to the callsite, and it looks like mostly we just log and then ignore them anyway. What do you think about adding failure callbacks to 437a0c2...stickies-v:bitcoin:pr/28051-use-cbs I think the first commit can be used ~as is, the second would probably have to be split across multiple existing commits. I think this PR could be an appropriate place to implement it, but otherwise (if you think the approach makes sense) I'm also happy to open a follow-up pull.
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. re: #28051 (comment)
This is intentional. The problem is that we have low-level code scattered in the codebase that is able to send shutdown signals, and this is not a good design. Lower level code should just be returning errors and notifications, and high level code should be using that information to decide whether to shut down, not reacting to shutdowns that could be triggered anywhere. The fact that bad code sending shutdown signals is more verbose now does not seem a problem, since code like this shoudl be discouraged and cleaned up anyway. This PR takes a step in that direction by removing the global Another thing this PR is trying to do is add appropriate error handling when sending shutdown signals fails, instead of crashing or ignoring the failures. The right way to handle these errors depends on where the errors happen. If an error happens in the
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
That does seem like a better design, indeed. I agree then that having the bad approach be more verbose is not something to worry about, and my suggestion should be discarded. Thank you for providing the rationale, I think this might be useful to add to the PR description?
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. re: #28051 (comment)
Definitely, and the PR description was pretty bad so I rewrote it. This is mentioned now at the end. |
||
| } | ||
| }, std::chrono::minutes{5}); | ||
|
|
||
|
|
@@ -1431,7 +1451,7 @@ bool AppInitMain(NodeContext& node, interfaces::BlockAndHeaderTipInfo* tip_info) | |
|
|
||
| // ********************************************************* Step 7: load block chain | ||
|
|
||
| node.notifications = std::make_unique<KernelNotifications>(node.exit_status); | ||
| node.notifications = std::make_unique<KernelNotifications>(*Assert(node.shutdown), node.exit_status); | ||
| ReadNotificationArgs(args, *node.notifications); | ||
| fReindex = args.GetBoolArg("-reindex", false); | ||
| bool fReindexChainState = args.GetBoolArg("-reindex-chainstate", false); | ||
|
|
@@ -1482,10 +1502,10 @@ 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>(node.kernel->interrupt, chainman_opts, blockman_opts); | ||
| node.chainman = std::make_unique<ChainstateManager>(*Assert(node.shutdown), chainman_opts, blockman_opts); | ||
| ChainstateManager& chainman = *node.chainman; | ||
|
|
||
| // This is defined and set here instead of inline in validation.h to avoid a hard | ||
|
|
@@ -1515,7 +1535,6 @@ bool AppInitMain(NodeContext& node, interfaces::BlockAndHeaderTipInfo* tip_info) | |
| options.check_blocks = args.GetIntArg("-checkblocks", DEFAULT_CHECKBLOCKS); | ||
| options.check_level = args.GetIntArg("-checklevel", DEFAULT_CHECKLEVEL); | ||
| options.require_full_verification = args.IsArgSet("-checkblocks") || args.IsArgSet("-checklevel"); | ||
| options.check_interrupt = ShutdownRequested; | ||
| options.coins_error_cb = [] { | ||
| uiInterface.ThreadSafeMessageBox( | ||
| _("Error reading from database, shutting down."), | ||
|
|
@@ -1550,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( | ||
|
|
@@ -1559,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; | ||
|
|
@@ -1573,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; | ||
| } | ||
|
|
@@ -1694,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; | ||
| } | ||
|
|
||
|
|
@@ -1714,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; | ||
| } | ||
|
|
||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -26,6 +26,11 @@ namespace node { | |
| struct NodeContext; | ||
| } // namespace node | ||
|
|
||
| /** Initialize node context shutdown and args variables. */ | ||
| void InitContext(node::NodeContext& node); | ||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Nit in the last commit: Shouldn't new node code be in the node namespace? Otherwise it will have to be touched again when it is moved into the node namespace later?
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. re: #28051 (comment)
This is true, but it is not different from other init.h methods. Logically init.h and init.cpp belong in node subdirectory and should use the node namespace, so I didn't want to make this one function stand out. The function also does need to be located in this file because it is accesses a static variable. So just keeping the Init functions in the same namespace seemed like the most straightforward approach for now. |
||
| /** Return whether node shutdown was requested. */ | ||
| bool ShutdownRequested(node::NodeContext& node); | ||
|
|
||
| /** Interrupt threads */ | ||
| void Interrupt(node::NodeContext& node); | ||
| void Shutdown(node::NodeContext& node); | ||
|
|
||
Uh oh!
There was an error while loading. Please reload this page.