Use a semaphore or pipe for shutdown notification (skeees, laanwj) #13211

1. 
   laanwj commented at 5:28 pm on May 10, 2018: member

   Instead of polling, use a semaphore (on WIN32) or a pipe (on POSIX) for shutdown notification. This avoids the need for a polling loop in bitcoind.

   Extends #13186.

   This does not change the polilng of ShutdownRequested() in the GUI. This is more complicated: on POSIX it would be possible to import the pipe[0] handle into the event loop, and call a handler when a character is detected, on Windows the StartShutdown() could make a callback that queues a qt signal at the GUI side. But I think that is better left for a later PR.

2. Use a semaphore or pipe for shutdown notification

   Instead of polling, use a semaphore (on WIN32) or a pipe (on POSIX) for
   shutdown notification. This avoids needing a polling loop in bitcoind.

   7191153bec
3. laanwj added the label Utils/log/libs on May 10, 2018
4. laanwj renamed this:
   Use a semaphore or pipe for shutdown notification
   Use a semaphore or pipe for shutdown notification (skeees, laanwj)
   on May 10, 2018
5. 
   skeees commented at 5:40 pm on May 10, 2018: contributor

   Oh cool - if you are going to split into platform dependent code - you can post to a pthreads semaphore from a unix signal handler. Would also suggest that anyone waiting/reading from the semaphore/pipe immediately post/write back that way you could have multiple threads listening for shutdown as opposed to restricting it to only one as you do currently
6. 
   laanwj commented at 5:43 pm on May 10, 2018: member

   you can post to a pthreads semaphore from a unix signal handler.

   I’m fairly sure none of the pthreads-api is signal safe, at least on all platforms.

   Would also suggest that anyone waiting/reading from the semaphore/pipe immediately post/write back that way you could have multiple threads listening for shutdown as opposed to restricting it to only one as you do currently

   We only need to wait for shutdown in one thread, so as I see it that additional complexity is not needed. If it happens to be needed in the future it can be added then.

   Edit: Travis issue is interesting though: looks like there is a race at shutdown where the HTTP server doesn’t get to send back the reply to stop(). I’d be surprised if it is new in this pull, though it probably comes to the foreground due to the instant shutdown notification. I remember previous discussion about this in #11006 (esp. #11006 (comment)).

7. Revert "Improve shutdown process"

   This reverts commit 793667af1c31835e0eefcdd283930bb89cfeda8f.

   12c7de1527
8. 
   laanwj commented at 6:17 pm on May 10, 2018: member

   Added a commit that reverts 793667a (#11006) to hopefully make travis pass. Likely, this has to be conditional on libevent version to prevent adding a delay on shutdown in another place while fixing another.

   Edit: that worked locally, but not on travis- weird.

9. 
   ken2812221 commented at 1:18 am on May 14, 2018: contributor

   Maybe that shutdown is too quick, add 50ms delay? It failed on my machine occasionally
10. 
    laanwj commented at 2:07 pm on June 4, 2018: member

    Maybe that shutdown is too quick, add 50ms delay? It failed on my machine occasionally

    Adding a fixed delay is never a valid solution.

11. 
    ken2812221 commented at 4:01 pm on June 16, 2018: contributor

    Once StopHTTPServer has been called in main thread ,the event loop in http thread exit. But http workqueue thread needs to use the event loop to call evhttp_send_reply, this could happen after the http thread event loop has already exit. https://github.com/bitcoin/bitcoin/blob/be27048a1842b76de5b230383c1466b72dfd8cc5/src/httpserver.cpp#L591
12. 
    laanwj commented at 4:32 pm on June 24, 2018: member

    Closing this for now, I don’t think it’s that important, the current way works fine.
13. laanwj closed this on Jun 24, 2018

    ---

14. MarcoFalke locked this on Sep 8, 2021

Contributors
laanwj skeees ken2812221

Labels
Utils/log/libs