[xyzconstant]

Suggested by @ryanofsky in #274 (comment) and #347 (comment)

---

Currently, synchronous calls from threads without an event loop are coordinated via a pipe. EventLoop::loop() blocks on the read end until work is scheduled via EventLoop::sync(). That method stores the function in m_sync_fn and writes a byte to the pipe, waking the loop to run that function. This is what allows other threads to perform I/O without an event loop.

But KJ already provides an API for cross-thread communication.: a const kj::Executor& obtained through kj::getCurrentThreadExecutor() and shared with other threads so they can schedule calls on the event loop.

This PR replaces the existing pipe-based sync-call scheduling with kj::Executor::executeSync.

It also uses kj::PromiseCrossThreadFulfillerPair<void> to wake EventLoop::loop() and let it exit. This wake-up is now only triggered from EventLoopRef::reset(), which before this PR was sharing m_post_writer with EventLoop::sync().

[DrahtBot]

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

Reviews

See the guideline and AI policy for information on the review process.

Type	Reviewers
Concept ACK	ryanofsky

If your review is incorrectly listed, please copy-paste <!--meta-tag:bot-skip--> into the comment that the bot should ignore.

Conflicts

Reviewers, this pull request conflicts with the following ones:

• #368 (proxy: defer thread-local client cleanup and worker joins by Sjors)
• #272 (ci: add Windows job by Sjors)
• #231 (Add windows support 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.

[xyzconstant]

Ran some numbers, comparing against master in a program that calls EventLoop::sync() from worker threads 50k times each and measures how long each batch of workers takes to finish.

Ran this on my M2 Pro, each variant in a dedicated worktree, built with -DCMAKE_BUILD_TYPE=Release -DCMAKE_CXX_FLAGS='-O3'. I ran it many times, and the results have been consistent with what is shown in this plot:

I find this interesting. The kj::Executor variant seems almost 2x faster than the pipe-based one.

NOTE: I'm not an expert at this benchmarking thing, and the setup is a bit awkward, so I'm just pasting the generated plot for now and will edit this comment with a Gist link later if/when I structure it better.

[xyzconstant]

I’ve been investigating the Bitcoin Core TSan data race failure. Here’s what’s happening, for anyone wondering:

Bear in mind that this PR enables cross-thread communication between KJ components for the first time. It exposed a conflict between atomic writes and non-atomic reads of the futex state (Linux only) owned by KJ's event-loop executor object. These non-atomic reads occur when capnproto is built in debug mode and have already been fixed upstream, but only in the v2 branch: capnproto/capnproto#2843.

At the libmultiprocess level, this issue occurs when:

1. A worker thread calling EventLoop::sync() goes through m_executor.executeSync() into the mutex wait, where a KJ_ASSERT in Mutex::assertLockedByCaller() performs a non-atomic read.

2. The event loop thread then wakes up inside the promise.wait() call in EventLoop::loop(), and Executor::poll() takes the same mutex with Mutex::lock(), which writes the futex word atomically.

This is harmless in practice because, despite both targeting the same futex word, they access different bits: the read end only reads the EXCLUSIVE_HELD bit, while the write end sets the EXCLUSIVE_REQUESTED bit.

The local sanitize job didn't catch this because capnproto is compiled in release mode, so the assertions are compiled out. In contrast, the Core job compiles capnp in debug mode. In the sanitize job, the nix compiler wrapper adds -O2. In the Core job, CXXFLAGS is passed to depends on the command line, which makes depends drop its default -O2. If no optimization flags are enabled, KJ then assumes it should build in debug mode.

I'm not sure if this is intentional, but we should decide whether to compile capnp in debug or release mode at both sites (in which case we must keep it consistent) or in one. Nevertheless, I'll be working on a backport PR to fix this upstream for v1 (master).

[ryanofsky]

Big concept ACK c16c467! This is a really nice simplification, but it will require careful review to make sure edge case conditions around previous done() and relock code are handled correctly. Also curious how you think we should work around the issue fixed in capnproto/capnproto#2855 so CI passes.

[ryanofsky]

Code review c16c467. Thanks for picking this up, and for the benchmark. It's nice that removing the pipe makes this both simpler and faster.

My main comments are about shutdown: loop() now exits the first time done() is true instead of re-checking it, and sync() no longer holds an EventLoopRef while its call is pending. Details and suggested fixes are in the threads below.

[ryanofsky]

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" (087b7b1)

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 0e4f88d) 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.

[ryanofsky]

re: #376 (comment)

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

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.

[ryanofsky]

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

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

[ryanofsky]

re: #376 (comment)

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.

[xyzconstant]

re:#376 (comment)

I am happy to work on this myself :)

[xyzconstant]

Opened a PR addressing this: bitcoin/bitcoin#36572

[ryanofsky]

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

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.

[ryanofsky]

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

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.

[ryanofsky]

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

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.

[ryanofsky]

re: #376 (comment)

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

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.

[xyzconstant]

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

[Sjors]

Does this affect the non-depends build?

[xyzconstant]

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

[xyzconstant]

Thanks for the review, @ryanofsky! I’ve just pushed some changes based on your feedback (compare), including squashing the two commits into one.

Regarding the TSan failure, I think the depends patch in Core is the right approach. I also believe libmultiprocess's sanitize job should build in debug mode to keep the libmultiprocess and Core jobs consistent, as mentioned in #376 (comment).