event-loop: replace pipe-based sync call scheduling with `kj::Executor::executeSync` #376

Update: I no longer think the change suggested here is needed. See the reply below.

In commit "loop: replace pipe-based sync call scheduling with kj::Executor::executeSync" (087b7b164e51f9b8592f7937c6be686fe586575e)

I think loop() should still check done() after it wakes up, and go back to waiting if it is false:

    for (;;) {
        m_stop_promise_fulfiller_pair.promise.wait(m_io_context.waitScope);
        const Lock lock(m_mutex);
        if (done()) break;
        // A new EventLoopRef or async cleanup was added after the promise was
        // fulfilled, so keep running and wait for the next fulfill() call.
        m_stop_promise_fulfiller_pair = kj::newPromiseAndCrossThreadFulfiller<void>();
    }

With the current version the promise is one-shot, so the loop exits the first time the refcount reaches 0, even if a new EventLoopRef was created or an async cleanup was queued before the loop thread woke up. Whatever holds that new ref would then point at an EventLoop that is about to be destroyed. The old loop re-checked done() with m_mutex held on every wakeup, so a transient 0 didn't stop it. That re-check was part of the fix in #129 for bitcoin/bitcoin#31151, where extra wakeups made the loop exit early. I don't see a current caller that lets the refcount come back from 0, but it would be good not to depend on that.

m_stop_promise_fulfiller_pair should also get MP_GUARDED_BY(m_mutex) with this change. reset() already only accesses it with m_mutex held.

For other reviewers wondering about the "Intentionally do not break if m_sync_fn was set" comment this removes: the race it described can't happen any more. The comment was added in #129 (commit 0e4f88d3f9c66c678cd56154e11827d5c9ec9f9e) to fix the "write(m_post_fd, &buffer, 1): Broken pipe" shutdown crash reported in bitcoin/bitcoin#31151, and kept the loop from destroying m_post_writer between another thread deciding to write to the pipe and the write. Now sync() has no shared writer, since KJ's XThreadEvent holds a reference to the Executor, so an executeSync() call racing with shutdown gets a DISCONNECTED exception instead of touching freed memory. And reset() calls fulfill() with m_mutex held, which loop() has to acquire before it returns.

re: #376 (review)

In commit "loop: replace pipe-based sync call scheduling with kj::Executor::executeSync" (087b7b164e51f9b8592f7937c6be686fe586575e)

Update: I don't think the re-check loop I suggested above is needed after all, and I've struck it out. Instead it would be good to document the rule that makes the current one-shot version safe, maybe on the loop() declaration:

    //! Run event loop. Does not return until shutdown. This should only be
    //! called once from the m_thread_id thread. This will block until the
    //! m_num_refs reference count drops to 0 and there are no pending async
    //! cleanup functions. This only happens once and loop() returns right
    //! after, so new EventLoopRef instances should only be created by code
    //! that already holds a reference, or is kept alive by one.

With callers following that rule, done() can't go from true back to false once loop() is running. The refcount can't come back from 0. m_async_fns can't be refilled while the count is 0, because addAsyncCleanup() is called from ProxyServerBase destructors while their own reference is still alive, and the async thread holds a reference while it runs each cleanup function. Dropping the count to 0 before loop() starts was never valid either, because done() would dereference the empty m_async_fns. So the re-check could only make a difference for a caller breaking the rule, and even then it would only narrow the race: a reference created from 0 just after the loop's last check would still point at a destroyed loop, as it would have with the old pipe loop.

The one place in libmultiprocess that creates references from 0 is the accept callback in _Listen(), which constructs a Connection from a plain EventLoop& (Listener doesn't hold a reference). That's correct as long as the ListenConnections() caller holds a reference while it wants to accept connections, which Bitcoin Core's CapnpProtocol::m_loop_ref and listen_tests.cpp both do. The listener can't hold the reference itself, because nothing removes it before the loop exits, so the loop would never exit. So I think that just needs documenting too, here or in a follow-up:

//! Given listening socket identifier and an init object, handle incoming
//! connections and requests by calling methods on the Init object.
//!
//! The listener does not hold an EventLoopRef, so the caller needs to hold
//! one for as long as connections should be accepted. Otherwise the event
//! loop exits after the last accepted connection is closed.

In commit "loop: replace pipe-based sync call scheduling with kj::Executor::executeSync" (087b7b164e51f9b8592f7937c6be686fe586575e)

Since sync() no longer holds its own EventLoopRef while the call is pending, it would be good to make the requirement on callers explicit in the sync() declaration in proxy-io.h. The current comment says it "Must be called while the loop() function is active", but what matters now is that loop() doesn't return before sync() does. If it did, executeSync() would block until ~EventLoop destroys m_io_context and then throw, fn would never run, and MP_LOG(*this, ...) here would use a destroyed m_log_opts. Maybe:

    //! Run function on event loop thread. Does not return until function completes.
    //! Must be called while the loop() function is active, and the caller must
    //! prevent loop() from returning until sync() returns, normally by holding
    //! an EventLoopRef.

I think relying on callers for this is fine. They all hold a ref already, through ProxyClient/ProxyServer contexts or m_loop_ref in Bitcoin Core's CapnpProtocol, and it avoids taking m_mutex on every call. (Edit: not all Bitcoin Core callers hold a reference in spawned processes, see reply below.)

re: #376 (review)

Correction: the last paragraph above is wrong about Bitcoin Core in spawned child processes, so I've struck out that part. This doesn't change the suggestion or affect this PR, but it's a Bitcoin Core bug worth fixing separately.

In CapnpProtocol::serve() (used by a bitcoin-node spawned by bitcoin-gui, for example), m_loop_ref is never set, and it can't be. serve() runs loop() on the child's main thread, and the loop has to exit when the parent disconnects so the child can exit. But m_loop_ref is only reset in ~CapnpProtocol, which can't run until serve() returns, so setting it would make the child hang. There, the parent connection's reference keeps the loop alive instead of m_loop_ref.

Calls that come from the parent are still fine, because they run inside an IPC request that holds a ProxyServer reference (for example Shutdown() calling disconnectIncoming() after the parent calls appShutdown()). Calls from the child's own threads are not covered. For example, the echoipc RPC handled on the child's RPC thread calls spawnProcess() → connect() → ConnectStream() → sync() without a reference. If the parent disconnects at that moment, the loop can exit and serve() can destroy m_loop while that thread is still using it. The reference the old sync() took didn't fully prevent this either, since m_loop itself is a std::optional reset by the main thread without synchronization.

The root of the problem is missing synchronization in CapnpProtocol. Once the parent is gone the child is shutting down anyway, so these calls should fail instead of keeping the loop alive. A fix could look like:

    //! Return a reference that keeps the event loop running, or throw if the
    //! loop is not running. Threads that are not handling an IPC request need
    //! this, because in serve() mode nothing else keeps the loop running for
    //! them.
    mp::EventLoopRef loopRef() EXCLUSIVE_LOCKS_REQUIRED(!m_loop_mutex)
    {
        LOCK(m_loop_mutex);
        if (!m_loop) throw Exception("IPC event loop is not running");
        mp::Lock lock(m_loop->m_mutex);
        if (m_loop->done()) throw Exception("IPC event loop is shutting down");
        mp::EventLoopRef ref{*m_loop, &lock};
        ref.m_lock = nullptr; // Lock m_mutex again when the reference is released.
        return ref;
    }

where a new m_loop_mutex is also held wherever m_loop is assigned or reset (serve(), startLoop()), and connect(), listen(), makeStream() and disconnectIncoming() call loopRef() and hold the result while they use the loop. Checking done() with m_mutex held is enough because done() never goes from true back to false once the loop is running (see the other thread). I can open a Bitcoin Core PR for this.

re:https://github.com/bitcoin-core/libmultiprocess/pull/376#discussion_r4268952676

I am happy to work on this myself :)

Opened a PR addressing this: bitcoin/bitcoin#36572

In commit "loop: replace pipe-based sync call scheduling with kj::Executor::executeSync" (087b7b164e51f9b8592f7937c6be686fe586575e)

Could you keep the comment from the old loop that explains why this logs and continues instead of rethrowing? Something like:

    // fn throwing is never expected. If it does happen, the caller of
    // EventLoop::sync() will return without any indication of failure, which
    // will likely cause other bugs. Log the error and continue.

Now that executeSync() delivers the exception on the calling thread, rethrowing would be possible, but some callers are destructors (~ProxyClient<Thread>), so logging still seems like the right default.

In commit "loop: drop stale conditional locking in EventLoopRef::reset()" (c16c467474e4484d893df15bb27ba9169bf9f4c7)

Could this comment say why it's safe to call fulfill() here? The comments this removes explained why the old pipe write couldn't race with the loop exiting, and the new reason is just as non-obvious. Maybe:

            // Wake up EventLoop::loop() so it can exit. This is called with
            // m_mutex held, and loop() locks m_mutex before returning, so the
            // loop can't exit and destroy the fulfiller while fulfill() is
            // running. Do not unlock m_mutex before this call.

In commit "loop: replace pipe-based sync call scheduling with kj::Executor::executeSync" (087b7b164e51f9b8592f7937c6be686fe586575e)

I'd suggest squashing the second commit into this one. In this commit reset() still unlocks m_mutex before fulfill() and optionally relocks it afterwards, and the comments here still refer to post_writer and to the "Intentionally do not break" comment that this commit deletes. The second commit is small, so reviewing the two together seems easier than reviewing this intermediate state.

re: #376 (comment)

In commit "loop: replace pipe-based sync call scheduling with kj::Executor::executeSync" (087b7b164e51f9b8592f7937c6be686fe586575e)

Thanks for tracking down the TSan failure in the Bitcoin Core job, and for getting the fix merged upstream in capnproto/capnproto#2855. The failure happens because KJ's debug assertions read the mutex futex word and AtomicRefcounted refcounts non-atomically. That only matters now that executeSync() makes KJ objects be used across threads, and it only shows up because this job builds capnp without optimization, which enables the assertions. Since no capnp release includes the fix yet, I think CI will need a workaround, and not just here: Bitcoin Core's own TSan job passes CXXFLAGS to depends the same way (ci/test/00_setup_env_native_tsan.sh), so it also builds KJ with debug assertions and would start failing once this change reaches the subtree.

The simplest fix I can see is to add the capnproto/capnproto#2855 diff as a capnp patch in Bitcoin Core's depends, next to the existing macos_accept_dead_socket.patch, and drop it when depends moves to a capnp release that includes the fix. The bitcoin-core-tsan job here builds depends from Bitcoin Core master, so it would pick up the patch once that is merged. TSan suppressions for kj::_::Mutex::assertLockedByCaller, kj::_::Mutex::unlock and kj::atomicAddRef would also work, but they would hide any real race reported in those frames, which the patch avoids.

I wouldn't switch depends to build capnp optimized in these jobs. Building it unoptimized does look accidental, but the debug assertions are useful in a sanitizer job, and they're what found this.

Opened bitcoin/bitcoin#36556 to add the capnproto/capnproto#2855 commits as capnp patches in depends.

Does this affect the non-depends build?

Not really, only when KJ's is built unoptimized. More details here: #376 (comment)