Use a condition variable for shutdown notifications #10873

1. 
   eklitzke commented at 11:33 pm on July 18, 2017: contributor

   Currently bitcoind polls every 200ms to see if a shutdown has been requested. This causes the process to wake up frequently and unnecessarily when the network is idle. The change here adds a condition variable so that the main bitcoind thread can do a blocking wait on the condition variable, rather than polling. Without this patch (i.e. in master) if you strace bitcoind you will see a lot of nanosleep() calls, with this patch the process does just a blocking wait on a futex(). The motivation here is that I want the CPU to be able to stay powered down more on my local bitcoin node.

   I elected to keep fRequestShutdown as an atomic variable, even though a new mutex is added for the condition variable, just to minimize code churn. I think there’s a case to be made that it should be turned into a regular bool and then ShutdownRequested() should use the mutex, but I don’t have a strong preference. To make it non-atomic and use the mutex would make the diff a little bigger, as there are a few places that read the atomic variable directly.

   This is my first Bitcoin diff. I have read the contributions doc, but please let me know if I made any errors related to code style, contributing, etc.

2. eklitzke force-pushed on Jul 18, 2017
3. eklitzke force-pushed on Jul 18, 2017
4. 
   sipa commented at 0:05 am on July 19, 2017: member

   @eklitzke Thanks for your contribution! I think your patch is pretty much acceptable as-is. Since the (new) fRequestShutdown is static and very localized, I would argue to convert it to a non-atomic in one go.

   As you’re pretty much rewriting all uses of these variables anyway, I think you could adapt them to the new coding style guidelines too (e.g. g_cs_shutdown, g_shutdown_condition, g_request_shutdown). That’s just a suggestion, not a requirement.

   Concept ACK

   Also note that we’re currently in the process of freezing for 0.15, merging this may be delayed a bit.

5. use a condition variable for shutdown notifications f684d2548c
6. eklitzke force-pushed on Jul 19, 2017
7. 
   jonasschnelli commented at 8:18 am on July 19, 2017: contributor

   Concept ACK.
8. jonasschnelli added the label Refactoring on Jul 19, 2017
9. in src/init.cpp:308 in f684d2548c

   304@@ -283,7 +305,7 @@ void Shutdown()
   305  */
   306 static void HandleSIGTERM(int)
   307 {
   308-    fRequestShutdown = true;
   309+    StartShutdown();
   

   ---

   ---

   

   laanwj commented at 2:21 pm on July 19, 2017:

   Unfortunately, a UNIX signal handler is not allowed to do anything besides set a flag. There is (on linux, but this differs per OS) a limited number of system calls that can be used, but this does not include POSIX threads stuff (see man 7 signal, Async-signal-safe functions).

   This is the only reason why we still have the polling.

   This SO answer might be useful: https://stackoverflow.com/a/31119139/216357

10. in src/init.cpp:136 in f684d2548c

    135+        g_request_shutdown = value;
    136+    }
    137+    g_shutdown_cond.notify_all();
    138+}
    139+
    140+void StartShutdown() {
    

    ---

    ---

    

    promag commented at 2:32 pm on July 19, 2017:

    Nit, new line for {.
11. in src/init.cpp:131 in f684d2548c

    130+static void SetShutdown(bool value)
    131 {
    132-    fRequestShutdown = true;
    133+    {
    134+        std::lock_guard<std::mutex> lk(g_shutdown_mut);
    135+        g_request_shutdown = value;
    

    ---

    ---

    

    promag commented at 2:42 pm on July 19, 2017:

    Add something like

    0if (g_request_shutdown == value) {
    1  // log something
    2  return;
    3}
    

12. 
    promag commented at 2:44 pm on July 19, 2017: member

    Concept ACK.
13. 
    laanwj commented at 12:21 pm on November 9, 2017: member

    Needs update for code review comments (and rebase)
14. 
    MarcoFalke commented at 8:07 pm on November 9, 2017: member

    This is up for grabs, it seems
15. 
    laanwj commented at 8:13 am on November 14, 2017: member

    I’m closing it for now, then. The UNIX signals + pthread issue is not trivial to resolve. Instead of a condition variable to synchronize you could use a file descriptor pair in a specific way that is allowed in a signal handler on any platform. I’m not up to date on the specifics, though.

    Let me know if you want to work on this again @eklitzke then we’ll reopen.

16. laanwj closed this on Nov 14, 2017

    ---

17. laanwj added the label Up for grabs on Nov 14, 2017
18. eklitzke deleted the branch on Mar 4, 2018
19. DrahtBot locked this on Sep 8, 2021

Contributors
eklitzke sipa jonasschnelli laanwj promag MarcoFalke

Labels
Refactoring Up for grabs