Bug 28

Server can hang at shutdown if a client connects as the last one leaves

Status

Fixed

Symptom

Hang; worst effect seen: hang

Where seen

Branch-build

Trigger

Shutdown

First reported

2025-06-10 (first report)

Fixed

2025-06-19

Fix

mp#160

Shipped to Bitcoin Core

bitcoin#32345

In released binaries

No; the code was in source releases only

Defect ever on master

Yes

Summary

An IPC server could hang at shutdown if a client connected just as the last one was going away. The event loop's async cleanup thread destroys server objects left behind by unclean disconnects. It exited for good as soon as it saw zero clients. Cleanup work queued by a connection that arrived afterwards was never run, so the event loop never finished and the process never exited. It was found through @darosior's connect/disconnect stress test with a Rust wallet client. No log of the hang was posted, and the fix's commit message is the only description of it. Source builds of v22 through v29 with multiprocess enabled have the defect. Fixed in mp#160, shipped in v30.0.

What happened

@darosior ran a Rust client that connected to and disconnected from bitcoin-node in a loop, testing bitcoin#32345 ("ipc: Handle unclean shutdowns better", then open) together with bitcoin#29409 ("multiprocess: Add capnp wrapper for Chain interface"), the branch that lets a wallet use the node over IPC. ryanofsky filed the results as mp#182 ("IPC crash with client disconnecting during IPC call", 2025-06-10), with logs. The visible problem in the logs was a crash in ~ProxyServerBase, which has its own page. According to the fix's commit message, the same loop also showed that shutdown "could hang waiting for cleanup work that will never be completed". The message of the next commit in the PR, 949573da8411, calls these "hangs on shutdown fixed by the previous commit".

Root cause

EventLoop::startAsyncThread (src/mp/proxy.cpp) runs a thread that destroys ProxyServer::m_impl objects left over from unclean disconnects. The thread exited as soon as the loop looked done. The initial commit's version broke out on m_num_clients == 0. A refactor earlier in mp#160 changed it to loop while (!done()) and break out when ref.reset() reported done, which has the same problem.

During shutdown the client count can reach zero and then rise again when the loop thread accepts one more incoming connection. If the async thread saw the momentary zero, it exited. startAsyncThread only starts a new thread when m_async_thread is not joinable, So when that last connection later disconnected, its cleanup functions were appended to m_async_fns, and notify_all() woke nobody. With m_async_fns non-empty, done() stayed false and EventLoop::loop() never returned. In bitcoin-node, CapnpProtocol holds its own EventLoopRef, so the count can only reach zero after Shutdown() has returned and ~CapnpProtocol releases it and joins the loop thread; that join is where the process would hang.

Both conditions need an external client: an unclean disconnect with live server objects, and a new connection at the moment the count reaches zero. Bitcoin Core's own spawned processes did neither.

The code

Quoted from ea38392960e1^, src/mp/proxy.cpp. The async cleanup thread keeps running while done() (no clients and no queued cleanups) is false, and exits as soon as releasing its own client reference makes done() true:

void EventLoop::startAsyncThread()
{
    if (m_async_thread.joinable()) {
        // Notify to wake up the async thread if it is already running.
        m_cv.notify_all();
    } else if (!m_async_fns.empty()) {
        m_async_thread = std::thread([this] {
            Lock lock(m_mutex);
            while (!done()) {
                if (!m_async_fns.empty()) {
                    EventLoopRef ref{*this, &lock};
                    const std::function<void()> fn = std::move(m_async_fns.front());
                    m_async_fns.pop_front();
                    Unlock(lock, fn);
                    // ...
                    if (ref.reset()) break;

done() being true only means the event loop is about to exit. During shutdown the loop thread can still accept one more incoming connection before it notices, which raises the client count again:

1. Async thread: finishes the last queued cleanup; ref.reset() drops the client count to 0, done() is true, and the thread exits.
2. Event loop: accepts a new connection before reading the wake-up byte, so done() is false again and the loop keeps running.
3. Event loop: the new client disconnects uncleanly; ~Connection appends its cleanups to m_async_fns and calls startAsyncThread(), which sees the exited thread is still joinable and only calls notify_all().
4. Nobody runs the queued cleanups, so m_async_fns stays non-empty, done() never becomes true, loop() never returns, and shutdown hangs joining the loop thread.

The fix (ea38392960e1) makes m_async_fns a std::optional that loop() engages when it starts and resets after it has exited, and runs the thread until then. The type change and the reset(relock) change that lets the thread keep the lock are omitted:

@@ -248,6 +256,8 @@ void EventLoop::loop()
     const Lock lock(m_mutex);
     m_wait_fd = -1;
     m_post_fd = -1;
+    m_async_fns.reset();
+    m_cv.notify_all();
 }
@@ -270,24 +280,21 @@ void EventLoop::post(kj::Function<void()> fn)
             Lock lock(m_mutex);
-            while (!done()) {
-                if (!m_async_fns.empty()) {
+            while (m_async_fns) {
+                if (!m_async_fns->empty()) {
                     EventLoopRef ref{*this, &lock};
-                    const std::function<void()> fn = std::move(m_async_fns.front());
-                    m_async_fns.pop_front();
+                    const std::function<void()> fn = std::move(m_async_fns->front());
+                    m_async_fns->pop_front();
                     Unlock(lock, fn);
-                    if (ref.reset()) break;
+                    // Important to relock because of the wait() call below.
+                    ref.reset(/*relock=*/true);

The thread now exits only after loop() has actually returned, when no new connection can be accepted and so no more cleanups can be queued; until then a later notify_all() always finds it waiting.

Fix

The fix commit ea38392960e1 ("Prevent EventLoop async cleanup thread early exit during shutdown"), in mp#160 ("refactor: EventLoop locking cleanups + client disconnect exception"), makes the thread wait for loop() to have exited instead of predicting that it is about to. m_async_fns becomes a std::optional<CleanupList>, engaged at the start of loop() and reset, with a notify_all(), after loop() finishes, and the thread runs while (m_async_fns). EventLoopRef::reset gets a relock flag so the thread can take the mutex again before waiting, and startAsyncThread asserts that it runs on the loop thread (+30/-22). No test targets the hang; the test added in mp#160 is for the ~ProxyServerBase crash.

Timeline

• 2019-06-25: the exit on zero clients present since the library's initial commit 06e1045baba0.
• 2025-06-10: mp#182 filed from @darosior's testing.
• 2025-06-19: fix merged in mp#160.
• 2025-08-18: shipped to Bitcoin Core in bitcoin#32345, which included a subtree update with mp#160; mp#182 closed 2025-08-23.
• Releases: v29.x and earlier have the defect in source builds with multiprocess enabled; v30.0 has the fix.

Notes

• ryanofsky called the new relock option a wart he would like to remove.

Classification

The survey's classification of this bug. The explorer charts these fields across all bugs; the method page explains them.