ipc: Handle unclean shutdowns better

ryanofsky commented at 8:58 pm on April 24, 2025: contributor

This PR fixes various problems when IPC connections are broken or hang which were reported in https://github.com/bitcoin-core/libmultiprocess/issues/123. The different fixes are described in commit messages.

This PR is a draft because it depends on https://github.com/bitcoin-core/libmultiprocess/pull/160. The non-base commits are:

This PR is part of the process separation project.

Squashed 'src/ipc/libmultiprocess/' changes from 35944ffd23fa..f15ef6cdeec5

f15ef6cdeec5 Improve IPC client disconnected exceptions
4a9a387e68b4 test: Add test coverage for client & server disconnections
a848ec60d490 refactor: Add clang thread safety annotations to EventLoop
4b97111ada84 refactor: Remove DestructorCatcher and AsyncCallable
276eb8f99d05 refactor: Drop addClient/removeClient methods
025a77ec2e46 refactor: Use EventLoopRef instead of addClient/removeClient
394f966e93f8 refactor: Add ProxyContext EventLoop* member
c1aa2d7dd546 proxy-io.h: Add EventLoopRef RAII class handle addClient/removeClient refcounting
2e02532f4720 proxy-io.h: Add more detailed EventLoop comment

git-subtree-dir: src/ipc/libmultiprocess
git-subtree-split: f15ef6cdeec54c83f52e00de47d57b09f9a5f03b

c021835739

Merge commit 'c021835739272d1daa8b4a2afc8e8c7d093d534c' into pr/ipc-stop-base b9e16ff790

ipc: Use EventLoopRef instead of addClient/removeClient

Use EventLoopRef to avoid reference counting bugs and be more exception safe
and deal with removal of addClient/removeClient methods in
https://github.com/bitcoin-core/libmultiprocess/pull/160

A test update is also required due to
https://github.com/bitcoin-core/libmultiprocess/pull/160 to deal with changed
reference count semantics. In IpcPipeTest(), it is is now necessary to destroy
the client Proxy object instead of just the client Connection object to
decrease the event loop reference count and allow the loop to exit so the test
does not hang on shutdown.

197b2aaaaa

test: Add unit test coverage for Init and Shutdown code

Currently this code is not called in unit tests. Calling should make it
possible to write tests for things like IPC exceptions being thrown during
shutdown.

cf1c26a3a2

ipc: Avoid waiting for clients to disconnect when shutting down

This fixes behavior reported by Antoine Poinsot <darosior@protonmail.com>
https://github.com/bitcoin/bitcoin/pull/29409#issuecomment-2546088852 where if
an IPC client is connected, the node will wait forever for it to disconnect
before exiting.

8ca7049cea

doc: Improve IPC interface comments

Fix some comments that were referring to previous versions of these methods and
did not make sense.

6427d6f175

ipc: Add Ctrl-C handler for spawned subprocesses

This fixes an error reported by Antoine Poinsot <darosior@protonmail.com> in
https://github.com/bitcoin-core/libmultiprocess/issues/123 that does not happen
in master, but does happen with https://github.com/bitcoin/bitcoin/pull/10102
applied, where if Ctrl-C is pressed when `bitcoin-node` is started, it is
handled by both `bitcoin-node` and `bitcoin-wallet` processes, causing the
wallet to shutdown abruptly instead of waiting for the node and shutting down
cleanly.

This change fixes the problem by having the wallet process print to stdout when
it receives a Ctrl-C signal but not otherwise react, letting the node shut
everything down cleanly.

db845b915f

ipc: Handle bitcoin-wallet disconnections

This fixes an error reported by Antoine Poinsot <darosior@protonmail.com> in
https://github.com/bitcoin-core/libmultiprocess/issues/123 that does not happen
in master, but does happen with https://github.com/bitcoin/bitcoin/pull/10102
applied, where if the child bitcoin-wallet process is killed (either by an
external signal or by Ctrl-C as reported in the issue) the bitcoin-node process
will not shutdown cleanly after that because chain client flush()
calls will fail.

This change fixes the problem by handling ipc::Exception errors thrown during
the flush() calls, and it relies on the fixes to disconnect detection
implemented in https://github.com/bitcoin-core/libmultiprocess/pull/160 to work
effectively.

5b38e62ccb

DrahtBot commented at 8:58 pm on April 24, 2025: contributor

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

Code Coverage & Benchmarks

For details see: https://corecheck.dev/bitcoin/bitcoin/pulls/32345.

Reviews

See the guideline for information on the review process.

Type	Reviewers
Concept ACK	darosior

If your review is incorrectly listed, please react with 👎 to this comment and the bot will ignore it on the next update.

Conflicts

Reviewers, this pull request conflicts with the following ones:

#32387 ([DRAFT] ipc: add windows support by ryanofsky)
#29641 (scripted-diff: Use LogInfo over LogPrintf [WIP, NOMERGE, DRAFT] by maflcko)

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.

LLM Linter (✨ experimental)

Possible typos and grammar issues:

FooImplemention → FooImplementation
explictly → explicitly

No other typos were found.

DrahtBot added the label CI failed on Apr 25, 2025

DrahtBot commented at 0:33 am on April 25, 2025: contributor

🚧 At least one of the CI tasks failed. Debug: https://github.com/bitcoin/bitcoin/runs/41118314270

Try to run the tests locally, according to the documentation. However, a CI failure may still happen due to a number of reasons, for example:

Possibly due to a silent merge conflict (the changes in this pull request being incompatible with the current code in the target branch). If so, make sure to rebase on the latest commit of the target branch.
A sanitizer issue, which can only be found by compiling with the sanitizer and running the affected test.
An intermittent issue.

Leave a comment here, if you need help tracking down a confusing failure.

fanquake commented at 10:20 am on April 25, 2025: member

https://github.com/bitcoin/bitcoin/actions/runs/14651550530/job/41119318297?pr=32345#step:5:966:

0Assertion failed: fRPCInWarmup, file ./rpc/server.cpp, line 329
1Error: Process completed with exit code 3.

https://cirrus-ci.com/task/5477140512112640?logs=ci#L5037:

0[17:33:37.941] ./test/node_init_tests.cpp(41): error: in "node_init_tests/init_test": check AppInitMain(m_node) has failed
1[17:33:37.941] Error: Cannot resolve -bind address: ''
2[17:33:37.954] ./test/node_init_tests.cpp(32): Leaving test case "init_test"; testing time: 32703us

https://cirrus-ci.com/task/5336403023757312?logs=ci#L2886:

0[18:45:09.651] /ci_container_base/src/ipc/interfaces.cpp:35:5: error: ignoring return value of function declared with 'warn_unused_result' attribute [-Werror,-Wunused-result]
1[18:45:09.651]    35 |     write(STDOUT_FILENO, g_ignore_ctrl_c.data(), g_ignore_ctrl_c.size());
2[18:45:09.651]       |     ^~~~~ ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
3[18:45:09.651] 1 error generated.

ryanofsky force-pushed on Apr 25, 2025

ryanofsky commented at 5:38 pm on April 25, 2025: contributor

Updated e8372fa3a606d35146c72fc61663e17c67a4621a -> a56468ab0bf2154159919a2c3feba9974486d10e (pr/ipc-stop.1 -> pr/ipc-stop.2, compare) to fix draftbot typos and CI failures, though I could not reproduce the previous releases AppInitMain failure in local CI so it could still be broken

darosior commented at 7:53 pm on April 25, 2025: member

Concept ACK. Will test using my Rust client for #29409 rebased on top of this.

ryanofsky force-pushed on Apr 28, 2025

ryanofsky commented at 4:22 pm on April 28, 2025: contributor

Updated a56468ab0bf2154159919a2c3feba9974486d10e -> 0b72656ecf363d9fc4820a9e3e22afb2c98d4e75 (pr/ipc-stop.2 -> pr/ipc-stop.3, compare) to try to fix init test CI failures https://github.com/bitcoin/bitcoin/actions/runs/14670251419/job/41176416988?pr=32345 and https://cirrus-ci.com/task/6094268256747520 Updated 0b72656ecf363d9fc4820a9e3e22afb2c98d4e75 -> 5b38e62ccbfcdb7f20a6a74fcc4ef2358e3da56c (pr/ipc-stop.3 -> pr/ipc-stop.4, compare) to fix test_bitcoin-qt failures caused by previous fix https://github.com/bitcoin/bitcoin/pull/32345/checks?check_run_id=41288589561

ryanofsky force-pushed on Apr 28, 2025

DrahtBot removed the label CI failed on Apr 28, 2025

maflcko commented at 6:18 am on April 29, 2025: member

There are also some typos, according to #32345 (comment), if you want to fix them if you have to re-touch for other reasons anyway.

ipc: Handle unclean shutdowns better #32345

Code Coverage & Benchmarks

Reviews

Conflicts

LLM Linter (✨ experimental)