bitcoind: Add -daemonwait option to wait for initialization #21007

pull laanwj wants to merge 3 commits into bitcoin:master from laanwj:2021-01-daemonwait changing 9 files +382 −57
  1. laanwj commented at 10:06 pm on January 25, 2021: member

    This adds a -daemonwait flag that does the same as -daemon except that it, from a user perspective, backgrounds the process only after initialization is complete. This is similar to the behaviour of some other software such as c-lightning.

    This can be useful when the process launching bitcoind wants to guarantee that either the RPC server is running, or that initialization failed, before continuing. The exit code indicates the initialization result.

    The use of the libc function daemon() is replaced by a custom implementation which is inspired by the glibc implementation, but which also creates a pipe from the child to the parent process for communication.

    An additional advantage of having our own daemon() implementation is that no MACOS-specific pragmas are needed anymore to silence a deprecation warning.

    TODO:

    • Factor out token_read and token_write to an utility, and use them in shutdown.cpp as well—this is exactly the same kind of communication mechanism.

      • RAII-ify pipe endpoints.
    • Improve granularity of the configure.ac checks. This currently still checks for the function daemon() which makes no sense as it’s not used. It should check for individual functions such as fork() and setsid() etc—the former being required, the second optional.

    • [-] Signal propagation during initialization: if say, pressing Ctrl-C during -daemonwait it would be good to pass this SIGINT on to the child process instead of detaching the parent process and letting the child run free. This is not necessary, see #21007 (comment).

    Future:

    • Consider if it makes sense to use this in the RPC tests (there would be no more need for “is RPC ready” polling loops). I think this is out of scope for this PR.
  2. laanwj added the label Utils/log/libs on Jan 25, 2021
  3. laanwj marked this as a draft on Jan 25, 2021
  4. laanwj force-pushed on Jan 25, 2021
  5. hebasto commented at 10:29 pm on January 25, 2021: member

    This can be useful when the process launching bitcoind wants to guarantee that either the RPC server is running, or that initialization failed, before continuing. The exit code indicates the initialization result.

    Why such behavior couldn’t be the default?

  6. in src/init.cpp:581 in f88aa35dfe outdated
    576@@ -577,7 +577,8 @@ void SetupServerArgs(NodeContext& node)
    577     argsman.AddArg("-server", "Accept command line and JSON-RPC commands", ArgsManager::ALLOW_ANY, OptionsCategory::RPC);
    578 
    579 #if HAVE_DECL_DAEMON
    580-    argsman.AddArg("-daemon", "Run in the background as a daemon and accept commands", ArgsManager::ALLOW_ANY, OptionsCategory::OPTIONS);
    581+    argsman.AddArg("-daemon", "Run in the background as a daemon and accept commands (default: 0)", ArgsManager::ALLOW_ANY, OptionsCategory::OPTIONS);
    582+    argsman.AddArg("-daemonwait", "Wait for initialization to be finished before exiting. This implies -daemon (default: 0)", ArgsManager::ALLOW_ANY, OptionsCategory::OPTIONS);
    


    hebasto commented at 10:32 pm on January 25, 2021:
    s/ALLOW_ANY/ALLOW_BOOL/ in both lines?

    laanwj commented at 7:28 am on January 26, 2021:
    Good idea! I’m also surprised there are no DEFAULT_XXX constants here.
  7. laanwj commented at 10:33 pm on January 25, 2021: member

    Why such behavior couldn’t be the default?

    I didn’t do that to avoid that discussion for now :slightly_smiling_face: . It could always be made default at some point in the future if that’s what people want.

  8. hebasto commented at 10:34 pm on January 25, 2021: member
    Concept ACK.
  9. DrahtBot commented at 2:08 am on January 26, 2021: member

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

    Conflicts

    Reviewers, this pull request conflicts with the following ones:

    • #21366 (refactor: replace util::Ref with std::any (C++17) by theStack)
    • #19471 (util: Make default arg values more specific by hebasto)
    • #19461 (multiprocess: Add bitcoin-gui -ipcconnect option by ryanofsky)
    • #19460 (multiprocess: Add bitcoin-wallet -ipcconnect option by ryanofsky)
    • #19160 (multiprocess: Add basic spawn and IPC support by ryanofsky)
    • #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.

  10. practicalswift commented at 9:37 am on January 26, 2021: contributor
    Concept ACK
  11. laanwj force-pushed on Jan 26, 2021
  12. laanwj force-pushed on Jan 26, 2021
  13. theStack commented at 6:57 pm on January 26, 2021: member
    Concept ACK
  14. laanwj force-pushed on Jan 26, 2021
  15. promag commented at 11:06 am on January 27, 2021: member

    Tested ACK 7482634cd455c6d25287e52795c2ac78cb1d44b0 on macos.

    With the following diff

     0--- a/src/init.cpp
     1+++ b/src/init.cpp
     2@@ -780,6 +780,9 @@ static bool InitSanityCheck()
     3
     4 static bool AppInitServers(const util::Ref& context, NodeContext& node)
     5 {
     6+    sleep(5);
     7+    return false;
     8+
     9     const ArgsManager& args = *Assert(node.args);
    10     RPCServer::OnStarted(&OnRPCStarted);
    11     RPCServer::OnStopped(&OnRPCStopped);
    

    On master

    0./src/bitcoind -regtest -daemon  && echo running
    1Bitcoin Core starting
    2running
    

    But it’s not running :trollface:

    And with this change:

    0./src/bitcoind -regtest -daemonwait && echo running
    1Bitcoin Core starting
    2Error during initializaton - check debug.log for details
    

    Good stuff, still have to review code change.

  16. laanwj commented at 1:57 pm on January 27, 2021: member

    Thanks for testing! FWIW what I use to force a failure in AppInitMain is

    0src/bitcoind -regtest -daemonwait -pid=/invalid/path
    

    (pid is, by definition, only written in the daemon process) But that works too :smile:

  17. hebasto commented at 2:01 pm on January 27, 2021: member

    (pid is, by definition, only written in the daemon process)

    Not sure if this is true in the master branch.

  18. laanwj commented at 2:11 pm on January 27, 2021: member

    Not sure if this is true in the master branch.

    I am sure of this. I did not change this at all, and besides, it would be a bug otherwise. After all, what use is the temporary parent process’ PID.

  19. hebasto commented at 2:17 pm on January 27, 2021: member

    … it would be a bug otherwise.

    0src/bitcoind -daemon=0
    

    creates bitcoind.pid

  20. laanwj commented at 2:24 pm on January 27, 2021: member

    creates bitcoind.pid

    That’s not my point. My point is that it creates the PID file after daemonizing (when daemon is enabled). Sure, it also creates the PID file otherwise, that’s okay. The only thing I was trying to say is that the option can be used for testing -daemonwait because it fails after daemonizing, nothing more.

  21. jonatack commented at 6:01 pm on January 27, 2021: member

    Tested at 7482634cd455c6d25287e52795c2ac78cb1d44b0, debug-builds cleanly for me and seems to behave as expected.

    invalid path

    0$ ./src/bitcoind -signet -daemonwait -pid=/invalid/path
    1Bitcoin Core starting
    2Error during initializaton - check debug.log for details
    

    (this particular case does not log anything to the debug log)

    happy path

    0$ ./src/bitcoind -signet -daemonwait ; ./src/bitcoin-cli -signet stop
    1Bitcoin Core starting
    2Bitcoin Core stopping
    

    debug log is as expected

     02021-01-27T17:49:33Z Command-line arg: daemonwait=""
     12021-01-27T17:49:33Z Command-line arg: signet=""
     22021-01-27T17:49:33Z Using at most 125 automatic connections (1024 file descriptors available)
     3...
     42021-01-27T17:49:35Z init message: Done loading
     52021-01-27T17:49:35Z dnsseed thread start
     62021-01-27T17:49:35Z Waiting 11 seconds before querying DNS seeds.
     72021-01-27T17:49:35Z tor: Thread interrupt
     82021-01-27T17:49:35Z addcon thread exit
     9...
    10
    112021-01-27T17:49:36Z Shutdown: done
    

    passing -daemonwait=1 behaves (and logs) as expected as well

    Ctrl-C works with a second or two of delay; it seems to not take effect until the wait is released but I’m not sure.

  22. laanwj commented at 6:13 pm on January 27, 2021: member

    Thanks for testing @jonatack ,

    (this particular case does not log anything to the debug log)

    That it doesn’t log anything is peculiar ! (but unrelated to this PR, I guess, the same problem would happen with -daemon, it would just mysteriously exit in the background).

    Ctrl-C works with a second or two of delay; it seems to not take effect until the wait is released but I’m not sure.

    It does respond to it? That’s interesting. I thought the child process wouldn’t get the SIGINT signal at all (hence my remaining TODO). That it doesn’t react immediately is normal, It can’t interrupt during verification of a block, for example. it’s the same when interrupting a non-daemon startup.

  23. jonatack commented at 8:23 pm on January 27, 2021: member

    Tested with testnet, which is much slower than signet for me (95 sec vs 3)

    0$ time ./src/bitcoind -testnet -daemonwait=1 && ./src/bitcoin-cli -testnet stop
    1Bitcoin Core starting
    2
    3real	1m35.265s
    4user	0m0.137s
    5sys	0m0.015s
    6Bitcoin Core stopping
    7$
    

    Ctrl-C right after launching interrupts successfully after the same time period elapses

    0$ time ./src/bitcoind -testnet -daemonwait=1
    1Bitcoin Core starting
    2^C
    3real	1m39.239s
    4user	0m0.130s
    5sys	0m0.022s
    6$
    
  24. laanwj commented at 12:01 pm on January 28, 2021: member

    I’ve looked into this a bit.

    It looks like that until the parent process exits, the child process is still attached to the terminal (or at least, shell session), so it still gets the SIGINT from Ctrl-C directly. Even though it did dup2 stdin/stdout/stderr to /dev/null already.

    So there appears to be no need to do explicit signal propagation. That’s neat. I’ll remove that TODO and open this for review.

  25. laanwj marked this as ready for review on Jan 28, 2021
  26. in src/util/tokenpipe.cpp:15 in 0ef284ba1a outdated
    10+
    11+TokenPipe::TokenPipe()
    12+{
    13+#if HAVE_O_CLOEXEC
    14+    if (pipe2(m_fds, O_CLOEXEC) != 0) {
    15+        return;
    


    theStack commented at 10:44 pm on February 2, 2021:
    Hm, it’s always a bit frustrating to see that within a C++ ctor it’s impossible to directly signal to the caller that the construction failed (I had to think about this article: https://250bpm.com/blog:4/). Since we can’t do anything immediately anyways if there was a failure, I guess the pipe{2} return value checks could simply be dropped?

    laanwj commented at 8:20 pm on February 3, 2021:

    I was first thinking of using the rust trick: to have a private constructor, and a static public factory function on the class that returns a std::optional<Pipe>. But it would complicate some things. E.g. currently this type is not movable at all, it felt like overkill to implement that just for initialization, but maybe it’s worth it if we can make it a standard idiom.

    Having to check complete() as a mandatory step feels slightly ugly too, after all.

  27. in src/util/tokenpipe.cpp:51 in 0ef284ba1a outdated
    46+{
    47+}
    48+
    49+TokenPipeEnd::~TokenPipeEnd()
    50+{
    51+    if (m_fd != -1) close(m_fd);
    


    theStack commented at 10:50 pm on February 2, 2021:
    Could simply call the Close() method here to deduplicate? (Only drawback: it unnecessarily sets m_fd to -1 before destroying the object.)

    laanwj commented at 8:26 pm on February 3, 2021:
    Yes, why not. It seemed like so little code that it wouldn’t really be worth deduplicating. I agree the redundant set to -1 doesn’t matter, this is not a performance critical function.
  28. in src/util/tokenpipe.h:20 in 0ef284ba1a outdated
    15+    int m_fd = -1;
    16+public:
    17+    TokenPipeEnd(int fd = -1);
    18+    ~TokenPipeEnd();
    19+
    20+    /** Return valus constants for TokenWrite and TokenRead. */
    


    theStack commented at 10:59 pm on February 2, 2021:
    0    /** Return value constants for TokenWrite and TokenRead. */
    
  29. in src/util/tokenpipe.h:26 in 0ef284ba1a outdated
    21+    enum Status {
    22+        TS_ERR = -1, //!< I/O error
    23+        TS_EOS = -2, //!< Unexpected end of stream
    24+    };
    25+
    26+    /** Read token from file descriptor.
    


    theStack commented at 11:01 pm on February 2, 2021:
    0    /** Write token to file descriptor.
    
  30. in src/bitcoind.cpp:199 in 7482634cd4 outdated
    202-                return InitError(Untranslated(strprintf("daemon() failed: %s\n", strerror(errno))));
    203+            switch (fork_daemon(1, 0, daemon_ep)) { // don't chdir (1), do close FDs (0)
    204+                case 0: // Child: continue.
    205+                    // If -daemonwait is not enabled, immediately send a success token the parent.
    206+                    if (!args.GetBoolArg("-daemonwait", DEFAULT_DAEMONWAIT)) {
    207+                        daemon_ep.TokenWrite(true);
    


    theStack commented at 11:17 pm on February 2, 2021:
    nit, feel free to ignore: maybe avoid implicit casting from bool to uint8_t and use literal 1 (or some character literal) as success token instead? I think as it is this could be potentially confusing for readers (also the counterpart TokenRead call below). As a drawback though, -daemonwait code gets longer: daemon_ep.TokenWrite(fRet ? 1 : 0).

    laanwj commented at 8:14 pm on February 3, 2021:
    Agree, it’s probably better to pass some constant. Or even just fRet as-is? It’s an integer after all. Never mind, i was already doing that, just turned this into a value.
  31. theStack commented at 11:40 pm on February 2, 2021: member
    Nice idea of introducing a RAII TokenPipe! This reduces the amount of low-level system calls / error handling code in the bitcoind module, looks much cleaner now. Left some review comments below, mostly nitpicking. Will review the the third commit (i.e. the actual introduction of -daemonwait) more in detail and also to test it on the weekend.
  32. laanwj force-pushed on Feb 16, 2021
  33. laanwj commented at 7:06 pm on February 16, 2021: member

    Addressed @theStack’s comments:

    • Added TokenPipe::create() that returns an optional (and make TokenPipe movable, and its constructor private), instead of the awkward incomplete construction.
    • Call Close() in destructor instead of duplicating code.
    • Consistently pass int to TokenWrite.
    • Fixed typos in comments.
  34. laanwj force-pushed on Feb 16, 2021
  35. in src/shutdown.cpp:37 in 581f48d818 outdated
    48-        return false;
    49-    }
    50-#endif
    51+    std::optional<TokenPipe> pipe = TokenPipe::create();
    52+    g_shutdown_r = pipe->ReadEnd();
    53+    g_shutdown_w = pipe->WriteEnd();
    


    theStack commented at 11:05 pm on February 17, 2021:
    The error-check after TokenPipe creation is missing here, e.g. if (!pipe) return false;

    laanwj commented at 8:52 am on February 18, 2021:
    Good point, will re-add an assertion. All the error handling in shutdown consists of assertions but skipping it is a bad idea. No, returning false is the right thing to do here.
  36. in src/bitcoind.cpp:49 in 48858432bc outdated
    44+int fork_daemon(bool nochdir, bool noclose, TokenPipeEnd &endpoint)
    45+{
    46+    // communication pipe with child process
    47+    std::optional<TokenPipe> umblical = TokenPipe::create();
    48+    if (!umblical) {
    49+        return -1; // pipe or pip2 failed.
    


    theStack commented at 11:06 pm on February 17, 2021:
    0        return -1; // pipe or pipe2 failed.
    
  37. in src/bitcoind.cpp:207 in 48858432bc outdated
    210+                    break;
    211+                case -1: // Error happened.
    212+                    return InitError(Untranslated(strprintf("fork_daemon() failed: %s\n", strerror(errno))));
    213+                default: { // Parent: wait and exit.
    214+                        int token = daemon_ep.TokenRead();
    215+                        if (token == true) { // fRet == true, Success
    


    theStack commented at 11:19 pm on February 17, 2021:

    nit:

    0                        if (token == 1) { // fRet == 1, Success
    

    (probably the token variable could be eliminated by calling TokenRead directly in the if condition.)


    laanwj commented at 8:53 am on February 18, 2021:
    I prefer having a variable here. Having a read as a side effect of an if() clause is kind of meh. Will change the true.
  38. in src/bitcoind.cpp:139 in 48858432bc outdated
    132@@ -59,6 +133,14 @@ static bool AppInit(int argc, char* argv[])
    133         return true;
    134     }
    135 
    136+#if HAVE_DECL_FORK
    137+    // Communication with parent after daemonizing. This is used for signalling in the following ways:
    138+    // - a boolean token is sent when the initialization process (all the Init* functions) have finished to indicate
    139+    // that the parent process can quit, and whether it was succesful/unsuccesful.
    


    theStack commented at 11:21 pm on February 17, 2021:

    typos:

    0    // that the parent process can quit, and whether it was successful/unsuccessful.
    
  39. in src/bitcoind.cpp:58 in 48858432bc outdated
    53+    if (pid < 0) {
    54+        return -1; // fork failed.
    55+    }
    56+    if (pid != 0) {
    57+        // Parent process gets read end, closes write end.
    58+        endpoint = umblical->ReadEnd();
    


    theStack commented at 11:29 pm on February 17, 2021:

    I didn’t know the word before (nice name for a parent-child-pipe 😄), but according to the dictionary the spelling is slightly different:

    0        endpoint = umbilical->ReadEnd();
    
  40. theStack commented at 11:35 pm on February 17, 2021: member
    Finished reviewing the code, LGTM. Left some comments, mostly nits and typos again.
  41. laanwj force-pushed on Feb 18, 2021
  42. promag commented at 1:54 am on February 21, 2021: member

    Tested ACK 652d4e4382.

    Should disallow conflict options like -daemon=false -daemonwait=true?

  43. laanwj commented at 12:40 pm on February 22, 2021: member

    Thanks for testing!

    Should disallow conflict options like -daemon=false -daemonwait=true?

    I’m not sure they’re really conflicting (“enable wait-for-daemon, but don’t daemonize”), though pointless, I see no reason to add a special error message for this.

  44. promag commented at 12:51 pm on February 22, 2021: member
    My point is with -daemon=false -daemonwait=true it will daemonize.
  45. laanwj commented at 1:12 pm on February 22, 2021: member

    My point is with -daemon=false -daemonwait=true it will daemonize.

    Oh that’s intentional, it means that you can just write bitcoind -daemonwait and it will work instead of having to do bitcoind -daemon -daemonwait.

  46. laanwj referenced this in commit 2e8116149c on Feb 23, 2021
  47. laanwj force-pushed on Feb 23, 2021
  48. laanwj commented at 6:07 pm on February 23, 2021: member
    Rebased for #21250.
  49. sidhujag referenced this in commit 0939a207f6 on Feb 24, 2021
  50. theStack approved
  51. theStack commented at 2:02 am on February 26, 2021: member
    Tested ACK 8e6d339434fdb6cfe5057b60f89ce08101e2eeb0 :beers: (ran on Debian GNU/Linux bullseye/sid, Kernel 4.19.0)
  52. in src/bitcoind.cpp:94 in 8e6d339434 outdated
    89+            bool err = dup2(fd, STDIN_FILENO) < 0 || dup2(fd, STDOUT_FILENO) < 0 || dup2(fd, STDERR_FILENO) < 0;
    90+            close(fd);
    91+            if (err) {
    92+                exit(1); // dup2 failed.
    93+            }
    94+            close(fd);
    


    ajtowns commented at 10:56 am on March 1, 2021:
    Why are you calling close(fd) twice? Shouldn’t it be a single if (fd > 2) close(fd); as per glibc?

    laanwj commented at 9:37 pm on March 1, 2021:
    Uhm, good catch, it should only be closed once. I don’t think there’s a need to compare it to 2.

    ajtowns commented at 4:22 am on March 2, 2021:
    The standard reason to compare it to 2 is in case you start the executable with stdin/stdout/stderr already closed – in that case fd will be assigned to 0/1/2 because that’s the first free fd, dup2(fd,stdin) (or out or err) will evaluate to dup2(0,0) and be a no-op, and you’ll then call close(0) which will leave you without a dummied stdin (or out or err) which will mean some later fd you open might overlap with stdin (or out or err). Not sure that things wouldn’t already be broken in those cases though – TokenPipe would probably grab the free fds in that case, and then dup2 would close the TokenPipe fd.

    laanwj commented at 12:12 pm on March 3, 2021:
    Hmm good point. I did not think about that case. It’s really unconventional to start binaries with stdin/stdout/stderr closed (the graceful way would be to redirect them to /dev/null and I’m not sure we handle this in other places.
  53. in src/bitcoind.cpp:71 in 8e6d339434 outdated
    66+
    67+        return pid;
    68+    }
    69+    // Child process gets write end, closes read end.
    70+    endpoint = umbilical->WriteEnd();
    71+    umbilical->ReadEnd();
    


    ajtowns commented at 11:07 am on March 1, 2021:
    Calling umbilical->ReadEnd() to create the TokenPipeEnd then immediately destroy it, which then closes the underlying fd seems a bit clunky. A comment might be worthwhile at least.

    laanwj commented at 9:27 pm on March 1, 2021:
    I thought about adding an explicit CloseReadEnd CloseWriteEnd function but as it’d essentially be the same I didn’t do so. Will add a comment.

    laanwj commented at 9:33 pm on March 1, 2021:
    What if I call the functions TakeReadEnd TakeWriteEnd would that make it clearer that a move is happening?

    ajtowns commented at 4:11 am on March 2, 2021:
    Could also write umbilical->ReadEnd().Close(); (relying on the destructor’s second call to Close then being a no-op). I don’t really have a preference.

    laanwj commented at 12:06 pm on March 3, 2021:

    Ah yea! I think I’ll do both.

    0umbilical->TakeReadEnd().Close();
    

    Fully self-documenting then.

  54. in src/util/tokenpipe.cpp:37 in 45fe328c1b outdated
    32+}
    33+
    34+void TokenPipe::Close()
    35+{
    36+    if (m_fds[0] != -1) close(m_fds[0]);
    37+    if (m_fds[1] != -1) close(m_fds[1]);
    


    ajtowns commented at 11:19 am on March 1, 2021:

    I think this would be incorrect if you had code like: { TokenPipe tp; a(); tp.Close(); b(); } in particular – the fds would be closed when Close() is called, but may be reallocated to different files during b() which would then be incorrectly closed when tp is destructed.

    Could explicitly set m_fds = {-1,-1}, or could drop Close() entirely and move the code into the destructor – it’s only called from operator=(TokenPipe&&) and you could just mark that as = deleted; I think; it doesn’t seem to be needed.


    laanwj commented at 9:28 pm on March 1, 2021:
    Will take a look. Agree the fds should be se to -1 after closing them to prevent ‘use fd after close’ problems, seems I forgot that. I made it like this to be symmetric with TokenPipeEnd::Close. Sure, I could move the code around if minimalism was the goal, but I like a consistent API more.
  55. in src/util/tokenpipe.cpp:14 in 45fe328c1b outdated
     9+
    10+#include <errno.h>
    11+#include <fcntl.h>
    12+#include <unistd.h>
    13+
    14+std::optional<TokenPipe> TokenPipe::create()
    


    ajtowns commented at 11:29 am on March 1, 2021:
    Doesn’t matter, but net_processing uses make as the name for PeerManager’s nearly-but-not-quite-a-constructor.

    laanwj commented at 9:29 pm on March 1, 2021:
    Thanks, I’m fine with a different name, create is the convention in rust but Make is fine with me too.

    ajtowns commented at 4:24 am on March 2, 2021:
    Yeah, either way. PeerManager could change too; it was just copied from some stackoverflow link or something.
  56. ajtowns commented at 11:30 am on March 1, 2021: member
    utACK 8e6d339434fdb6cfe5057b60f89ce08101e2eeb0
  57. in src/Makefile.am:243 in 8e6d339434 outdated
    239@@ -240,6 +240,7 @@ BITCOIN_CORE_H = \
    240   util/memory.h \
    241   util/message.h \
    242   util/moneystr.h \
    243+  util/tokenpipe.h \
    


    jonatack commented at 11:35 am on March 1, 2021:
    nit, sort

    laanwj commented at 12:37 pm on March 3, 2021:
    Strange, I guess I had this module named differently at first :smile:
  58. in src/util/tokenpipe.cpp:67 in 8e6d339434 outdated
    62+}
    63+
    64+int TokenPipeEnd::TokenWrite(uint8_t token)
    65+{
    66+    while (true) {
    67+        int result = write(m_fd, &token, 1);
    


    jonatack commented at 11:52 am on March 1, 2021:

    write seems to be returning ssize_t / long, same for read line 86

    0        const long result{write(m_fd, &token, 1)};
    

    laanwj commented at 9:59 pm on March 1, 2021:
    good catch with the type, not sure though i like the {} initialization style more here, = seems to express intent clearer
  59. in src/init.h:15 in 8e6d339434 outdated
     8@@ -9,6 +9,11 @@
     9 #include <memory>
    10 #include <string>
    11 
    12+//! Default value for -daemon option
    13+static const bool DEFAULT_DAEMON = false;
    14+//! Default value for -daemonwait option
    15+static const bool DEFAULT_DAEMONWAIT = false;
    


    jonatack commented at 11:53 am on March 1, 2021:
    these two can be constexpr
  60. in src/bitcoind.cpp:89 in 8e6d339434 outdated
    84+    if (!noclose) {
    85+        // Open /dev/null, and clone it into STDIN, STDOUT and STDERR to detach
    86+        // from terminal.
    87+        int fd = open("/dev/null", O_RDWR);
    88+        if (fd >= 0) {
    89+            bool err = dup2(fd, STDIN_FILENO) < 0 || dup2(fd, STDOUT_FILENO) < 0 || dup2(fd, STDERR_FILENO) < 0;
    


    jonatack commented at 11:54 am on March 1, 2021:
    fd and err can be const

    laanwj commented at 12:41 pm on March 3, 2021:
    I’m not really sure what the common thing is to do for internal variables, do we have a const-unless-required-otherwise recommendation?

    jonatack commented at 12:58 pm on March 3, 2021:
    Yes, I don’t think it’s explicitly stated, other than maybe via the C++ Code Guidelines linked to in the developer notes, but it’s what I’ve been doing/reviewing, and seeing others say and do.
  61. in src/shutdown.cpp:84 in 8e6d339434 outdated
    90-            assert(errno == EINTR);
    91-        } else {
    92-            assert(result == 1);
    93-            break;
    94-        }
    95+    int res = g_shutdown_r.TokenRead();
    


    jonatack commented at 11:55 am on March 1, 2021:
    res here and line 54 can be const
  62. jonatack commented at 11:56 am on March 1, 2021: member

    tACK 8e6d339434fdb6cfe5057b60f89ce08101e2eeb0

    Some nit suggestions below, feel free to ignore.

  63. in src/util/tokenpipe.cpp:55 in 8e6d339434 outdated
    50+    m_fds[1] = -1;
    51+    return res;
    52+}
    53+
    54+TokenPipeEnd::TokenPipeEnd(int fd):
    55+    m_fd(fd)
    


    hebasto commented at 6:09 pm on March 1, 2021:

    style-nit, suggested by clang-format-diff.py:

    0TokenPipeEnd::TokenPipeEnd(int fd)
    1    : m_fd(fd)
    
  64. in src/util/tokenpipe.h:19 in 8e6d339434 outdated
    12+
    13+/** One end of a token pipe. */
    14+class TokenPipeEnd {
    15+private:
    16+    int m_fd = -1;
    17+public:
    


    hebasto commented at 6:10 pm on March 1, 2021:

    style-nit, suggested by clang-format-diff.py:

    0class TokenPipeEnd
    1{
    2private:
    3    int m_fd = -1;
    4
    5public:
    
  65. in src/util/tokenpipe.h:67 in 8e6d339434 outdated
    62+        m_fd = other.m_fd;
    63+        other.m_fd = -1;
    64+        return *this;
    65+    }
    66+    TokenPipeEnd (const TokenPipeEnd&) = delete;
    67+    TokenPipeEnd &operator=(const TokenPipeEnd&) = delete;
    


    hebasto commented at 6:11 pm on March 1, 2021:

    style-nit, suggested by clang-format-diff.py:

    0    TokenPipeEnd(const TokenPipeEnd&) = delete;
    1    TokenPipeEnd& operator=(const TokenPipeEnd&) = delete;
    
  66. in src/util/tokenpipe.h:73 in 8e6d339434 outdated
    68+};
    69+
    70+/** An interprocess or interthread pipe for sending tokens (one-byte values)
    71+ * over.
    72+ */
    73+class TokenPipe {
    


    hebasto commented at 6:12 pm on March 1, 2021:

    style-nit, suggested by clang-format-diff.py:

    0class TokenPipe
    1{
    
  67. in src/util/tokenpipe.h:82 in 8e6d339434 outdated
    73+class TokenPipe {
    74+private:
    75+    int m_fds[2] = {-1, -1};
    76+
    77+    TokenPipe(int fds[2]): m_fds{fds[0], fds[1]} {}
    78+public:
    


    hebasto commented at 6:13 pm on March 1, 2021:

    style-nit, suggested by clang-format-diff.py:

    0    TokenPipe(int fds[2]) : m_fds{fds[0], fds[1]} {}
    1
    2public:
    
  68. in src/util/tokenpipe.h:103 in 8e6d339434 outdated
     98+    void Close();
     99+
    100+    // Move-only class.
    101+    TokenPipe(TokenPipe&& other)
    102+    {
    103+        for(int i = 0; i < 2; ++i) {
    


    hebasto commented at 6:13 pm on March 1, 2021:

    style-nit, suggested by clang-format-diff.py:

    0        for (int i = 0; i < 2; ++i) {
    
  69. in src/util/tokenpipe.h:111 in 8e6d339434 outdated
    106+        }
    107+    }
    108+    TokenPipe& operator=(TokenPipe&& other)
    109+    {
    110+        Close();
    111+        for(int i = 0; i < 2; ++i) {
    


    hebasto commented at 6:14 pm on March 1, 2021:

    style-nit, suggested by clang-format-diff.py:

    0        for (int i = 0; i < 2; ++i) {
    
  70. in src/util/tokenpipe.h:118 in 8e6d339434 outdated
    113+            other.m_fds[i] = -1;
    114+        }
    115+        return *this;
    116+    }
    117+    TokenPipe (const TokenPipe&) = delete;
    118+    TokenPipe &operator=(const TokenPipe&) = delete;
    


    hebasto commented at 6:14 pm on March 1, 2021:

    style-nit, suggested by clang-format-diff.py:

    0    TokenPipe(const TokenPipe&) = delete;
    1    TokenPipe& operator=(const TokenPipe&) = delete;
    

    laanwj commented at 9:51 pm on March 1, 2021:
    Thanks. FWIW it’s fine to just mark one of these and say to run clang-format-diff, no need to open a review item for every little indentation/style mismatch :smile: (as this makes it harder to find the more serious comments)
  71. in src/bitcoind.cpp:44 in 8e6d339434 outdated
    39+ *          -1 in case of error (in parent process).
    40+ *
    41+ *          In case of success, endpoint will be one end of a pipe from the child to parent process,
    42+ *          which can be used with TokenWrite (in the child) or TokenRead (in the parent).
    43+ */
    44+int fork_daemon(bool nochdir, bool noclose, TokenPipeEnd &endpoint)
    


    hebasto commented at 6:16 pm on March 1, 2021:

    style-nit, suggested by clang-format-diff.py:

    0int fork_daemon(bool nochdir, bool noclose, TokenPipeEnd& endpoint)
    
  72. in src/bitcoind.cpp:213 in 8e6d339434 outdated
    216+                            exit(EXIT_SUCCESS);
    217+                        } else { // fRet = false or token read error (premature exit).
    218+                            tfm::format(std::cout, "Error during initializaton - check debug.log for details\n");
    219+                            exit(EXIT_FAILURE);
    220+                        }
    221+                    }
    


    hebasto commented at 6:22 pm on March 1, 2021:

    indentation-nit, suggested by clang-format-diff.py:

     0            switch (fork_daemon(1, 0, daemon_ep)) { // don't chdir (1), do close FDs (0)
     1            case 0: // Child: continue.
     2                // If -daemonwait is not enabled, immediately send a success token the parent.
     3                if (!args.GetBoolArg("-daemonwait", DEFAULT_DAEMONWAIT)) {
     4                    daemon_ep.TokenWrite(1);
     5                    daemon_ep.Close();
     6                }
     7                break;
     8            case -1: // Error happened.
     9                return InitError(Untranslated(strprintf("fork_daemon() failed: %s\n", strerror(errno))));
    10            default: // Parent: wait and exit.
    11                int token = daemon_ep.TokenRead();
    12                if (token) { // Success
    13                    exit(EXIT_SUCCESS);
    14                } else { // fRet = false or token read error (premature exit).
    15                    tfm::format(std::cout, "Error during initializaton - check debug.log for details\n");
    16                    exit(EXIT_FAILURE);
    17               }
    
  73. in src/util/tokenpipe.h:84 in 8e6d339434 outdated
    79+    ~TokenPipe();
    80+
    81+    /** Create a new pipe.
    82+     * @returns The created TokenPipe, or an empty std::nullopt in case of error.
    83+     */
    84+    static std::optional<TokenPipe> create();
    


    hebasto commented at 6:24 pm on March 1, 2021:

    Capitalize function name as others?

    0    static std::optional<TokenPipe> Create();
    
  74. in src/util/tokenpipe.h:121 in 8e6d339434 outdated
    116+    }
    117+    TokenPipe (const TokenPipe&) = delete;
    118+    TokenPipe &operator=(const TokenPipe&) = delete;
    119+};
    120+
    121+#endif
    


    hebasto commented at 6:24 pm on March 1, 2021:
    0#endif // WIN32
    
  75. hebasto commented at 6:25 pm on March 1, 2021: member
    Approach ACK 8e6d339434fdb6cfe5057b60f89ce08101e2eeb0
  76. laanwj force-pushed on Mar 3, 2021
  77. laanwj commented at 12:58 pm on March 3, 2021: member
    @hebasto @jonatack @ajtowns Thanks for the reviews. I have addressed your comments.
  78. in src/bitcoind.cpp:210 in b83b386c6e outdated
    213+            default: { // Parent: wait and exit.
    214+                int token = daemon_ep.TokenRead();
    215+                if (token) { // Success
    216+                    exit(EXIT_SUCCESS);
    217+                } else { // fRet = false or token read error (premature exit).
    218+                    tfm::format(std::cout, "Error during initializaton - check debug.log for details\n");
    


    jonatack commented at 4:28 pm on March 3, 2021:
    b83b386 s/cout/cerr/?

    laanwj commented at 5:02 pm on March 3, 2021:
    Yes, why not
  79. in src/util/tokenpipe.cpp:92 in b83b386c6e outdated
    103+{
    104+    if (m_fd != -1) close(m_fd);
    105+    m_fd = -1;
    106+}
    107+
    108+#endif
    


    jonatack commented at 4:31 pm on March 3, 2021:

    0b9994ac

    0#endif // WIN32
    
  80. in src/bitcoind.cpp:38 in b83b386c6e outdated
    33+
    34+/** Custom implementation of daemon(). This implements the same order of operations as glibc.
    35+ * Opens a pipe to the child process to be able to wait for an event to occur.
    36+ *
    37+ * @returns 0 if succesful, and in child process.
    38+ *          >0 if succesful, and in parent process.
    


    jonatack commented at 4:33 pm on March 3, 2021:
    b83b386c lines 37 and 38: s/succesful/successful/
  81. jonatack commented at 4:36 pm on March 3, 2021: member

    ACK b83b386c6e03a57f93110dfefdbbf515e62b5753 reviewed diff/code, built/tested

    Some nits if you retouch.

  82. laanwj force-pushed on Mar 4, 2021
  83. laanwj commented at 2:14 pm on March 4, 2021: member
  84. jonatack commented at 4:09 pm on March 4, 2021: member
    re-ACK 9a09a494e59b61d7a185da44f9c6736cb536c0c1
  85. util: Add RAII TokenPipe 612f746a8f
  86. shutdown: Use RAII TokenPipe in shutdown c3e6fdee6d
  87. bitcoind: Add -daemonwait option to wait for initialization
    This adds a `-daemonwait` flag that does the same as `-daemon` except
    it, from a user perspective, backgrounds the process only after
    initialization is complete.
    
    This can be useful when the process launching bitcoind wants to
    guarantee that either the RPC server is running, or that initialization
    failed, before continuing. The exit code indicates the initialization
    result.
    
    This replaces the use of the libc function `daemon()` by a custom
    implementation which is inspired by the glibc implementation, but also
    creates a pipe from the child to the parent process for communication.
    
    An additional advantage of having our own `daemon()` implementation is
    that no MACOS-specific pragmas are needed anymore to silence a
    deprecation warning.
    e017a913d0
  88. laanwj force-pushed on Mar 4, 2021
  89. laanwj commented at 5:27 pm on March 4, 2021: member
    Repushed for an ordering issue in tokenpipe.cpp reported out-of-band, the implementation file now has same class order as the header 9a09a494e59b61d7a185da44f9c6736cb536c0c1..e017a913d0d78ef0766cf73586fe7a38488e1a26
  90. jonatack commented at 6:23 pm on March 4, 2021: member

    Tested ACK e017a913d0d78ef0766cf73586fe7a38488e1a26 checked change since previous review is move-only

     0$ time ./src/bitcoind -testnet -daemonwait=1 && ./src/bitcoin-cli -testnet stop
     1Bitcoin Core starting
     2
     3real	1m4.095s
     4user	0m0.132s
     5sys	0m0.027s
     6Bitcoin Core stopping
     7
     8$ time ./src/bitcoind -testnet -daemonwait=1
     9Bitcoin Core starting
    10^C 
    11real	1m5.311s
    12user	0m0.047s
    13sys	0m0.023s
    
  91. laanwj merged this on Mar 11, 2021
  92. laanwj closed this on Mar 11, 2021

  93. laanwj added the label Needs release note on Mar 11, 2021
  94. sidhujag referenced this in commit 51927eeb04 on Mar 11, 2021
  95. laanwj referenced this in commit 01bb3afb51 on Mar 16, 2021
  96. sidhujag referenced this in commit 24c1d91006 on Mar 16, 2021
  97. PastaPastaPasta referenced this in commit f6a2ab5365 on Sep 17, 2021
  98. whitslack referenced this in commit 49287e0ab4 on Jan 14, 2022
  99. DrahtBot locked this on Aug 16, 2022
  100. fanquake removed the label Needs release note on Sep 15, 2022
  101. fanquake commented at 3:14 pm on September 15, 2022: member
    This change was part of 22.0, removing “Needs release note”.

github-metadata-mirror

This is a metadata mirror of the GitHub repository bitcoin/bitcoin. This site is not affiliated with GitHub. Content is generated from a GitHub metadata backup.
generated: 2024-10-04 13:12 UTC

This site is hosted by @0xB10C
More mirrored repositories can be found on mirror.b10c.me