init: Signal-safe instant shutdown #20605

laanwj · 2020-12-08T21:14:28Z

Replace the 200ms polling loop with a faster and more efficient waiting operation. This should speed up short RPC tests.

This change has been tried a few times before, but abandoned every time because solutions used 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.

This only affects bitcoind. The GUI is unaffected by this change, and keeps polling as before in BitcoinGUI::detectShutdown(). It might be possible to listen to a pipe there, too, but I'm not sure, and it's complicated by the GUI-node abstraction.

sipa · 2020-12-08T21:19:59Z

That's an amazing hack. I didn't know it was allowed to call system library functions from a signal handler, but apparently POSIX mandates that some functions are callable - including read() and write().

DrahtBot · 2020-12-09T06:49:14Z

The following sections might be updated with supplementary metadata relevant to reviewers and maintainers.

Conflicts

Reviewers, this pull request conflicts with the following ones:

Add syscall sandboxing using seccomp-bpf (Linux secure computing mode) #20487 (draft: Add syscall sandboxing using seccomp-bpf (Linux secure computing mode) by practicalswift)
multiprocess: Add bitcoin-gui -ipcconnect option #19461 (multiprocess: Add bitcoin-gui -ipcconnect option by ryanofsky)
multiprocess: Add bitcoin-wallet -ipcconnect option #19460 (multiprocess: Add bitcoin-wallet -ipcconnect option by ryanofsky)
multiprocess: Add basic spawn and IPC support #19160 (multiprocess: Add basic spawn and IPC support by ryanofsky)
Multiprocess bitcoin #10102 ([experimental] Multiprocess bitcoin by ryanofsky)

If you consider this pull request important, please also help to review the conflicting pull requests. Ideally, start with the one that should be merged first.

jonasschnelli · 2020-12-09T08:09:14Z

Nice!
Tested ACK f8585d5 - (on macOS only)

Is there a documentation what system function one can call in signal handler (to confirm with POSIX)?

laanwj · 2020-12-09T09:26:24Z

Is there a documentation what system function one can call in signal handler (to confirm with POSIX)?

This is a good question, I wasn't able to find any definitive reference. The "self-pipe trick" is an incredibly common pattern in UNIX daemons though. It certainly works across Linux, Macos, modern BSDs… Fairly sure everything that can run Bitcoin Core. If it would be broken in principle then a lot of software would break.

For example Python uses this too, calls it a 'wakeup fd'. See https://github.com/python/cpython/blob/master/Modules/signalmodule.c#L294

promag

Concept ACK.

promag · 2020-12-09T11:36:21Z

src/shutdown.cpp

+        // 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);


I've tried locally to just close() and it also works (tested on macos only). Note that WaitForShutdown is called twice so just one read succeeds, see promag@c44a33c.

What do you mean with "WaitForShutdown is called twice"? It isn't. It's called at the end of AppInit which itself is called once.

Or if you mean the call in AbortShutdown, sure, yes but that's on purpose to reset the shutdown condition so it can be used again.
I'd really prefer to use read/write and not close a socket under someone waiting on it !

Ah right, the second call is In AbortShutdown. But that doesn't invalidate the point of just close() instead of write(), which looks enough to make read() return.

"looks enough"
Yes, but we're dealing with a lot of possibilities in different OSes here, just because it works for you doesn't mean it's safe. This whole thing (try googling about signals and pipe tricks) is a controversial subject, so I prefer to mimic existing code instead of inventing our own.
Also, close() makes it more work to re-arm the condition in AbortShutdown.

Edit: To be fair I do think it's a clever trick to use close, and it might be good in some circumstances, but probably not in this case.

jonatack · 2020-12-09T11:53:13Z

Tested ACK eae384f on Debian 5.9.11-1 (2020-11-27) x86_64 GNU/Linux

promag · 2020-12-09T12:20:02Z

Tested ACK eae384f on macos 10.15.6, needs squash I guess.

laanwj · 2020-12-09T12:25:26Z

Squashed f8585d5..eae384f → f628398

jonatack

Tested re-ACK f628398 on Debian 5.9.11-1 (2020-11-27) x86_64 GNU/Linux

Timed runs of a short rpc tests compared to master, they appear faster, and manual bitcoind shutdowns seem snappier.

decryp2kanon · 2020-12-10T06:39:31Z

Concept ACK ❤️

practicalswift · 2020-12-10T09:29:54Z

Concept ACK: swift shutdown is more practical than non-swift shutdown

luke-jr · 2020-12-14T16:22:53Z

On Windows, there are no signals so using a condition variable is safe.

But Windows does have signals...?

laanwj · 2020-12-14T16:59:20Z

But Windows does have signals...?

But not asynchronous ones like in POSIX. Windows always uses event loops.

Edit: okay this is apparently not true, some notifications launch in a separate thread. It seems they are not reentrant in the same sense as POSIX signals though.

laanwj · 2020-12-14T17:38:36Z

~~Closing this, might pick it up some day or someone else could.~~

Edit: we only set a SetConsoleCtrlHandler on Windows and this launches in a separate thread so this is exactly the right appreach.

sipa · 2020-12-14T17:48:17Z

I suggest reopening, the approach here looks correct for Windows: https://docs.microsoft.com/en-us/windows/console/console-control-handlers (the control handlers are run in a separate thread).

re: bitcoin#20605 (comment) > This only affects bitcoind. The GUI is unaffected by this change, and keeps polling as before in `BitcoinGUI::detectShutdown()`. It might be possible to listen to a pipe there, too, but I'm not sure, and it's complicated by the GUI-node abstraction.

ryanofsky

Code review ACK f628398. Seems like a good approach. Could be cleaned up various ways (could be wrapped in a util class, could close file descriptors before shutdown, could handle more runtime errors and avoid asserts) but none of this seems important. I have a question below about AbortShutdown that would be good to clear up, but at worst I think may be a minor bug there, and wouldn't want to hold up the PR.

re: #20605 (comment)

This only affects bitcoind. The GUI is unaffected by this change, and keeps polling as before in BitcoinGUI::detectShutdown(). It might be possible to listen to a pipe there, too, but I'm not sure, and it's complicated by the GUI-node abstraction.

The GUI is already asynchronous so shouldn't need any polling/piping/condition variable stuff. Just some signals boilerplate like 73be44d (branch)

ryanofsky · 2020-12-14T21:53:02Z

src/shutdown.cpp

+/** 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];


In commit "init: Signal-safe instant shutdown" (f628398)

Could set some initial values:

static int g_shutdown_pipe[2] = {-1, -1};

To avoid possibility of code reading and writing to random pipes if someone forgets to call InitShutdownState.

Good idea.
(0 is STDIN_FILENO so it should be more or less safe, but still, it'd be surprising behavior, it's better to raise an error)

ryanofsky · 2020-12-14T22:34:29Z

src/shutdown.cpp

+    if (fRequestShutdown) {
+        // Cancel existing shutdown by waiting for it, this will reset condition flags and remove
+        // the shutdown token from the pipe.
+        WaitForShutdown();


In commit "init: Signal-safe instant shutdown" (f628398)

I don't understand what AbortShutdown is supposed to do. Why would a caller request and abort a shutdown instead of just not requesting a shutdown until it wanted the shutdown? What prevents AppInit from exiting between the StartShutdown and AbortShutdown calls and causing AbortShutdown to hang forever? I could be misunderstanding, but it seems like AbortShutdown functionality was racy before this PR, and now racy and deadlocky after this PR.

I'm happy to see it go but it's a) used in the tests b) before WaitForShutdown() is called in init. Both uses are safe, and I mention this in the comment (in the header file).

re: #20605 (comment)

I'm happy to see it go but it's a) used in the tests b) before WaitForShutdown() is called in init. Both uses are safe, and I mention this in the comment (in the header file).

Thanks for pointing out the comment. I missed that this was meant to be called before WaitForShutdown, so this makes much more sense now!

laanwj · 2020-12-15T16:00:55Z

The GUI is already asynchronous so shouldn't need any polling/piping/condition variable stuff. Just some signals boilerplate like 73be44d (branch)

Wouldn't raising a Qt signal also, likely, be unsafe in a signal handler?

could handle more runtime errors and avoid asserts

How do you suggest handling them? I used asserts because the program is in an unknown state if any of the conditions happen, it should only happen due to a coding bug or memory corruption, and there's not a sane way to continue, e.g. either we spend forever waiting for shutdown, or the shutdown sequence runs prematurely (thinking of it, the latter is safe? then again it would be unexpected, at least with an assert it's easier to see where something went wrong).

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.

laanwj · 2020-12-15T16:22:44Z

Added initializer for g_shutdown_pipe.
f628398 → cd03513

jonatack · 2020-12-15T22:52:43Z

ACK cd03513 tested on Debian 5.9.11-1 (2020-11-27) x86_64 GNU/Linux

When I read while (true) for some reason I think of 20 GOTO 10 or JMP ($000A)

laanwj · 2020-12-16T09:54:24Z

When I read while (true) for some reason I think of 20 GOTO 10 or JMP ($000A)

Heh, it could definitely be done without that, but I tend to write retry loops (especially those with complicated exit conditions, though that's not the case here) as infinite loops usually. Here it avoids having to define result outside the loop and think about what to initialize it with for the first iteration.

cd03513 init: Signal-safe instant shutdown (Wladimir J. van der Laan) Pull request description: Replace the 200ms polling loop with a faster and more efficient waiting operation. This should speed up short RPC tests. This change has been tried a few times before, but abandoned every time because solutions used 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. This only affects bitcoind. The GUI is unaffected by this change, and keeps polling as before in `BitcoinGUI::detectShutdown()`. It might be possible to listen to a pipe there, too, but I'm not sure, and it's complicated by the GUI-node abstraction. ACKs for top commit: jonatack: ACK cd03513 tested on Debian 5.9.11-1 (2020-11-27) x86_64 GNU/Linux Tree-SHA512: ed2f532f69fec4855c17bf7b8f3d0eb96e78ee2a3c13d374dd2c6add06e3ad6a190da8ed8f9d7a76532cf998222d67f57e35b206aec29675e96437448ae7e13c

ryanofsky

Post-merge code review ACK cd03513. Only change since last review initializing descriptor values.

re: #20605 (comment)

Wouldn't raising a Qt signal also, likely, be unsafe in a signal handler?

Oh! You're completely right according to https://doc.qt.io/qt-5/unix-signals.html. So do need a thread. This can be added to previous implementation with small tweak: b6b5674 (pr/guishut.2)

I think potentional good followups for this PR would be:

Removing polling from GUI. Sketched above.
Moving platform specific pipe / condition variable code into an RAII util class that closes file descriptors and can be unit tested

ryanofsky · 2020-12-17T13:06:22Z

src/shutdown.cpp

+    if (fRequestShutdown) {
+        // Cancel existing shutdown by waiting for it, this will reset condition flags and remove
+        // the shutdown token from the pipe.
+        WaitForShutdown();


re: #20605 (comment)

I'm happy to see it go but it's a) used in the tests b) before WaitForShutdown() is called in init. Both uses are safe, and I mention this in the comment (in the header file).

Thanks for pointing out the comment. I missed that this was meant to be called before WaitForShutdown, so this makes much more sense now!

laanwj added the Utils/log/libs label Dec 8, 2020

laanwj force-pushed the 2020_12_instant_shutdown branch from 67fd692 to f8585d5 Compare December 8, 2020 21:19

DrahtBot mentioned this pull request Dec 9, 2020

Add syscall sandboxing using seccomp-bpf (Linux secure computing mode) #20487

Merged

promag reviewed Dec 9, 2020

View reviewed changes

DrahtBot mentioned this pull request Dec 9, 2020

multiprocess: Add basic spawn and IPC support #19160

Merged

laanwj force-pushed the 2020_12_instant_shutdown branch from eae384f to f628398 Compare December 9, 2020 12:25

jonatack reviewed Dec 9, 2020

View reviewed changes

This was referenced Dec 11, 2020

Multiprocess bitcoin #10102

Draft

multiprocess: Add bitcoin-wallet -ipcconnect option #19460

Draft

multiprocess: Add bitcoin-gui -ipcconnect option #19461

Draft

laanwj closed this Dec 14, 2020

laanwj reopened this Dec 14, 2020

ryanofsky approved these changes Dec 14, 2020

View reviewed changes

laanwj force-pushed the 2020_12_instant_shutdown branch from f628398 to cd03513 Compare December 15, 2020 16:22

laanwj merged commit 3f20580 into bitcoin:master Dec 16, 2020

ryanofsky reviewed Dec 17, 2020

View reviewed changes

bitcoin locked as resolved and limited conversation to collaborators Feb 15, 2022

init: Signal-safe instant shutdown #20605

init: Signal-safe instant shutdown #20605

Uh oh!

Conversation

laanwj commented Dec 8, 2020

Uh oh!

sipa commented Dec 8, 2020

Uh oh!

DrahtBot commented Dec 9, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Conflicts

Uh oh!

jonasschnelli commented Dec 9, 2020

Uh oh!

laanwj commented Dec 9, 2020

Uh oh!

promag left a comment

Choose a reason for hiding this comment

Uh oh!

promag Dec 9, 2020

Choose a reason for hiding this comment

Uh oh!

laanwj Dec 9, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

laanwj Dec 9, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

promag Dec 9, 2020

Choose a reason for hiding this comment

Uh oh!

laanwj Dec 9, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jonatack commented Dec 9, 2020

Uh oh!

promag commented Dec 9, 2020

Uh oh!

laanwj commented Dec 9, 2020

Uh oh!

jonatack left a comment

Choose a reason for hiding this comment

Uh oh!

decryp2kanon commented Dec 10, 2020

Uh oh!

practicalswift commented Dec 10, 2020

Uh oh!

luke-jr commented Dec 14, 2020

Uh oh!

laanwj commented Dec 14, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

laanwj commented Dec 14, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

sipa commented Dec 14, 2020

Uh oh!

ryanofsky left a comment

Choose a reason for hiding this comment

Uh oh!

ryanofsky Dec 14, 2020

Choose a reason for hiding this comment

Uh oh!

laanwj Dec 15, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ryanofsky Dec 14, 2020

Choose a reason for hiding this comment

Uh oh!

laanwj Dec 15, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ryanofsky Dec 17, 2020

Choose a reason for hiding this comment

Uh oh!

DrahtBot commented Dec 9, 2020 •

edited

Loading

laanwj Dec 9, 2020 •

edited

Loading

laanwj Dec 9, 2020 •

edited

Loading

laanwj Dec 9, 2020 •

edited

Loading

laanwj commented Dec 14, 2020 •

edited

Loading

laanwj commented Dec 14, 2020 •

edited

Loading

laanwj Dec 15, 2020 •

edited

Loading

laanwj Dec 15, 2020 •

edited

Loading

laanwj commented Dec 15, 2020 •

edited

Loading