Merge #20605: init: Signal-safe instant shutdown

cd03513dc2 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 cd03513dc2 tested on Debian 5.9.11-1 (2020-11-27) x86_64 GNU/Linux

Tree-SHA512: ed2f532f69fec4855c17bf7b8f3d0eb96e78ee2a3c13d374dd2c6add06e3ad6a190da8ed8f9d7a76532cf998222d67f57e35b206aec29675e96437448ae7e13c

src/bitcoind.cpp

@@ -28,15 +28,6 @@
 const std::function<std::string(const char*)> 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;

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."));

src/shutdown.cpp

@@ -5,19 +5,108 @@
 
 #include <shutdown.h>
 
+#include <config/bitcoin-config.h>
+
+#include <assert.h>
 #include <atomic>
+#ifdef WIN32
+#include <condition_variable>
+#else
+#include <errno.h>
+#include <fcntl.h>
+#include <unistd.h>
+#endif
 
 static std::atomic<bool> 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<std::mutex> 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<std::mutex> 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
+}

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