bitcoind: Add -daemonwait option to wait for initialization

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.

laanwj added the label Utils/log/libs on Jan 25, 2021

laanwj marked this as a draft on Jan 25, 2021

laanwj force-pushed on Jan 25, 2021

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?

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.

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.

hebasto commented at 10:34 pm on January 25, 2021: member

Concept ACK.

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.

practicalswift commented at 9:37 am on January 26, 2021: contributor

Concept ACK

laanwj force-pushed on Jan 26, 2021

theStack commented at 6:57 pm on January 26, 2021: member

Concept ACK

laanwj force-pushed on Jan 26, 2021

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.

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:

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.

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.

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

… it would be a bug otherwise.

0src/bitcoind -daemon=0

creates bitcoind.pid

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.

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.

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.

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$

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.

laanwj marked this as ready for review on Jan 28, 2021

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.

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.

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. */

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.

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.

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.

laanwj force-pushed on Feb 16, 2021

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.

laanwj force-pushed on Feb 16, 2021

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.

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.

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.

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.

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();

theStack commented at 11:35 pm on February 17, 2021: member

Finished reviewing the code, LGTM. Left some comments, mostly nits and typos again.

laanwj force-pushed on Feb 18, 2021

laanwj commented at 12:18 pm on February 18, 2021: member

Thanks again for the review @theStack Pushed https://github.com/bitcoin/bitcoin/compare/48858432bc7ef3e02685b693954173e722bae4e5..652d4e4382e8964db3e1ba6e4bb8247daad7ae64

promag commented at 1:54 am on February 21, 2021: member

Tested ACK 652d4e4382.

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

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.

promag commented at 12:51 pm on February 22, 2021: member

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

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.

laanwj referenced this in commit 2e8116149c on Feb 23, 2021

laanwj force-pushed on Feb 23, 2021

laanwj commented at 6:07 pm on February 23, 2021: member

Rebased for #21250.

sidhujag referenced this in commit 0939a207f6 on Feb 24, 2021

theStack approved

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)

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.

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.

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.

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.

ajtowns commented at 11:30 am on March 1, 2021: member

utACK 8e6d339434fdb6cfe5057b60f89ce08101e2eeb0

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:

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

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

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.

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

jonatack commented at 11:56 am on March 1, 2021: member

tACK 8e6d339434fdb6cfe5057b60f89ce08101e2eeb0

Some nit suggestions below, feel free to ignore.

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)

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:

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;

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{

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:

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) {

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) {

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)

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)

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               }

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();

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

hebasto commented at 6:25 pm on March 1, 2021: member

Approach ACK 8e6d339434fdb6cfe5057b60f89ce08101e2eeb0

laanwj force-pushed on Mar 3, 2021

laanwj commented at 12:58 pm on March 3, 2021: member

@hebasto @jonatack @ajtowns Thanks for the reviews. I have addressed your comments.

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

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

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/

jonatack commented at 4:36 pm on March 3, 2021: member

ACK b83b386c6e03a57f93110dfefdbbf515e62b5753 reviewed diff/code, built/tested

Some nits if you retouch.

laanwj force-pushed on Mar 4, 2021

laanwj commented at 2:14 pm on March 4, 2021: member

Thanks @jonatack, have addressed the comments and re-pushed b83b386c6e03a57f93110dfefdbbf515e62b5753..9a09a494e59b61d7a185da44f9c6736cb536c0c1

jonatack commented at 4:09 pm on March 4, 2021: member

re-ACK 9a09a494e59b61d7a185da44f9c6736cb536c0c1

util: Add RAII TokenPipe 612f746a8f

shutdown: Use RAII TokenPipe in shutdown c3e6fdee6d

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

laanwj force-pushed on Mar 4, 2021

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

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

laanwj merged this on Mar 11, 2021

laanwj closed this on Mar 11, 2021

laanwj added the label Needs release note on Mar 11, 2021

sidhujag referenced this in commit 51927eeb04 on Mar 11, 2021

laanwj referenced this in commit 01bb3afb51 on Mar 16, 2021

sidhujag referenced this in commit 24c1d91006 on Mar 16, 2021

PastaPastaPasta referenced this in commit f6a2ab5365 on Sep 17, 2021

whitslack referenced this in commit 49287e0ab4 on Jan 14, 2022

DrahtBot locked this on Aug 16, 2022

fanquake removed the label Needs release note on Sep 15, 2022

fanquake commented at 3:14 pm on September 15, 2022: member

This change was part of 22.0, removing “Needs release note”.

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

Conflicts