Add nonunix platform support by ryanofsky · Pull Request #274 · bitcoin-core/libmultiprocess

ryanofsky · 2026-04-22T21:53:45Z

This PR implements API changes and fixes needed to allow libmultiprocess to work on nonunix platforms.

These changes were originally part of #231, which adds windows support, but were split out to allow windows and nonwindows changes to be reviewed separately.

DrahtBot · 2026-04-22T21:54:07Z

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
Stale ACK	Sjors, enirox001

If your review is incorrectly listed, please copy-paste  into the comment that the bot should ignore.

Conflicts

Reviewers, this pull request conflicts with the following ones:

#296 (ci: Bump channel to nixos-26.05 by maflcko)
#209 (cmake: Increase cmake policy version by ryanofsky)
#175 (Set cmake_minimum_required(VERSION 3.22) 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:

nonunix -> non-Unix [misspelling/word form; the intended platform name is clearer with “Unix” capitalized and hyphenated]
It safe to access post_writer here... -> It is safe to access post_writer here... [missing verb makes the sentence grammatically broken]

^{2026-07-10 23:18:34}

ryanofsky

Thanks for the review! Will update with suggestions.

Sjors · 2026-06-17T14:23:06Z

I opened stratum-mining/sv2-tp#110 to test the changes in the Template Provider.

Sjors

Studied two first three commits...

Sjors

Reviewed the rest as well, except the CMake changes in c9aa806 and 7cb83a5.

Concept ACK

Sjors · 2026-06-18T17:29:46Z

 //! errors in python unit tests.
 std::string LogEscape(const kj::StringTree& string, size_t max_size);

+using Stream = kj::Own<kj::AsyncIoStream>;


In 091f5e1 proxy, refactor: Change ConnectStream and ServeStream to accept stream objects: would it make sense to introduce this earlier, in 3c81cf2?

re: #274 (comment)

In 091f5e1 proxy, refactor: Change ConnectStream and ServeStream to accept stream objects: would it make sense to introduce this earlier, in 3c81cf2?

It could make sense but I feel it's a little clearer if the EventLoop code is using kj::Own<kj::AsyncIoStream> and kj::Own<kj::OutputStream> types directly and not tied to the mp::Stream type for external callers and meant to be more opaque.

But I did extend this commit to use Stream stream type in ConnectStream and ServeStream functions since these are external functions.

In 174604a "proxy, refactor: Change ConnectStream and ServeStream to accept stream objects"

Had a concern that the name Stream is fairly broad, so using it too widely could hide useful information at call sites. For example, inside EventLoop it seems helpful to see the concrete KJ type details directly, otherwise future code might start using mp::Stream everywhere just because the alias exists.

I agree with keeping it limited to external facing methods like ConnectStream and ServeStream, where the abstraction is useful, and avoiding it in internal code where the underlying type is more informative.

re: #274 (comment)

Agreed, glad the reasoning made sense!

ryanofsky

Thanks for the reviews!

Rebased 7cb83a5 -> 68ed129 (pr/wins.1 -> pr/wins.2, compare) implementing review suggestions and fixing conflict with #279

Updated 68ed129 -> a43e5a8 (pr/wins.2 -> pr/wins.3, compare) to fix environ undeclared on macOS/BSD: add explicit declaration before posix_spawn() https://github.com/bitcoin-core/libmultiprocess/actions/runs/27974097183/job/82787313836

Updated a43e5a8 -> 6cc729f (pr/wins.3 -> pr/wins.4, compare) to fix clang-tidy readability-redundant-declaration on environ declaration https://github.com/bitcoin-core/libmultiprocess/actions/runs/27978804911/job/82803323635

ryanofsky · 2026-06-22T16:28:08Z

 //! errors in python unit tests.
 std::string LogEscape(const kj::StringTree& string, size_t max_size);

+using Stream = kj::Own<kj::AsyncIoStream>;


re: #274 (comment)

In 091f5e1 proxy, refactor: Change ConnectStream and ServeStream to accept stream objects: would it make sense to introduce this earlier, in 3c81cf2?

It could make sense but I feel it's a little clearer if the EventLoop code is using kj::Own<kj::AsyncIoStream> and kj::Own<kj::OutputStream> types directly and not tied to the mp::Stream type for external callers and meant to be more opaque.

But I did extend this commit to use Stream stream type in ConnectStream and ServeStream functions since these are external functions.

Sjors

ACK 6cc729f

I still didn't review the cmake commits though.

Sjors · 2026-06-23T16:14:47Z

-        }
-        _exit(1);
+    ProcessId pid;
+    if (int err = posix_spawn(&pid, argv[0], nullptr, nullptr, argv.data(), ::environ)) {


In ddd3945 util, refactor: Do not fork() and exec() separately: not posix_spawnp?

re: #274 (comment)

In ddd3945 util, refactor: Do not fork() and exec() separately: not posix_spawnp?

Good catch! Switched to posix_spawnp here (so PATH) is still used

Sjors · 2026-06-23T16:36:03Z


+//! Create a socket pair that can be used to communicate within a process or
+//! between parent and child processes.
+std::array<SocketId, 2> SocketPair();


In d9a5a68 util, refactor: Add SocketPair() and use it in SpawnProcess: it seems a bit odd to set FD_CLOEXEC and then unset it again for the child. The unset code also assume it was the only flag, since it sets 0. Maybe it's better to give this function a boolean argument for whether to set this flag on the child?

re: #274 (comment)

In d9a5a68 util, refactor: Add SocketPair() and use it in SpawnProcess: it seems a bit odd to set FD_CLOEXEC and then unset it again for the child. The unset code also assume it was the only flag, since it sets 0. Maybe it's better to give this function a boolean argument for whether to set this flag on the child?

Good catch on assuming no other flags were set, this should be fixed now by calling F_GETFD. It still seems a little less error prone to explicitly unset FD_CLOEXEC right before forking, instead of adding an option and treating the socketpair fds differently

39a10ce proxy: add local connection limit to ListenConnections() (Enoch Azariah) 43172f5 test: add dedicated ListenConnections coverage (Enoch Azariah) 033f812 doc/version: Bump version 11 > 12 (Enoch Azariah) Pull request description: This adds an optional local connection limit to`ListenConnections()`. Previously, `ListenConnections()` would accept incoming connections indefinitely. This branch adds an optional `max_connections` parameter so a listener can stop accepting new connections once a per-listener cap is reached, and resume accepting when an existing connection disconnects. The limit is local to the listener instead of global to the `EventLoop`. This keeps the state and behavior scoped to the listening socket, and is closer to the direction discussed downstream for per-`-ipcbind` limits. This also adds a test covering the behavior with `max_connections=1`, verifying that: - the first client is accepted normally - a second client is not accepted while the first remains connected - the second client is accepted after the first disconnects _Note This PR includes a major version bump to `v12` due to the API addition. If #274 lands earlier and bumps the version to `v12` first, we will need to bump the version here again._ ACKs for top commit: ryanofsky: Code review ACK 39a10ce Tree-SHA512: 3334056c8161a8de6681650e8377e441377a29cf7e754af7f0d9419aee1165f0828906ca70dff1aeca90e4496cf600d36cbba01bf0f48c56c7cac8a54a159446

enirox001

Code Review ACK c5faf68

Initial look at the chnages implemented here

Left a few suggestions and questions inline.

enirox001 · 2026-07-07T08:50:43Z

In 0ec1534 ("util, refactor: Add SocketId type alias and use it"):

Should this parameter be using SocketId instead of int to maintain type consistency with the rest of the code?

index 91d02cc..29bcb54 100644 --- a/include/mp/proxy-io.h +++ b/include/mp/proxy-io.h @@ -876,14 +876,14 @@ void ServeStream(EventLoop& loop, Stream stream, InitImpl& init) _Serve<InitInterface>(loop, kj::mv(stream), init); } -//! Given listening socket file descriptor and an init object, handle incoming +//! Given listening socket identifier and an init object, handle incoming //! connections and requests by calling methods on the Init object. template <typename InitInterface, typename InitImpl> -void ListenConnections(EventLoop& loop, int fd, InitImpl& init) +void ListenConnections(EventLoop& loop, SocketId socket_id, InitImpl& init) { loop.sync([&]() { _Listen<InitInterface>(loop, - loop.m_io_context.lowLevelProvider->wrapListenSocketFd(fd, kj::LowLevelAsyncIoProvider::TAKE_OWNERSHIP), + loop.m_io_context.lowLevelProvider->wrapListenSocketFd(socket_id, kj::LowLevelAsyncIoProvider::TAKE_OWNERSHIP), init); }); }

re: #274 (comment)

Good catch, added SocketId here

enirox001 · 2026-07-07T15:26:25Z

-        std::cerr << argv[1] << " is not a number or is larger than an int\n";
-        return 1;
-    }
+    mp::SocketId socket{mp::StartSpawned(argv[1])};


In c7a939b "util, refactor: Add SpawnConnectInfo type alias and use it"

Didn't we lose our error handling here? std::stoi throws exceptions on bad input, so shouldn't we add a try-catch to prevent a crash?

re: #274 (comment)

Didn't we lose our error handling here? std::stoi throws exceptions on bad input, so shouldn't we add a try-catch to prevent a crash?

Yes previous error handling provided more context. Changed to catch and throw a more descriptive exception. It can be up to callers whether to crash in this case (which probably indicates a bug not a runtime error) and crashing in the examples seems ok

enirox001 · 2026-07-08T15:51:40Z

 //! errors in python unit tests.
 std::string LogEscape(const kj::StringTree& string, size_t max_size);

+using ProcessId = int;


in 5f578bb "util, refactor: Add ProcessId type alias and use it"

nit: Since this refactor adds ProcessId, should we also add ProcessError{-1} and use it for the fork() failure check, matching SocketId and SocketError

index f647d5d..05e1b4b 100644 --- a/include/mp/util.h +++ b/include/mp/util.h @@ -276,6 +276,7 @@ using Stream = kj::Own<kj::AsyncIoStrea m>; using ProcessId = int; using SocketId = int; constexpr SocketId SocketError{-1}; +constexpr ProcessId ProcessError{-1}; //! Information about parent process passed to child proce ss as a command-line //! argument. On unix this is the child socket fd number f ormatted as a string. diff --git a/src/mp/util.cpp b/src/mp/util.cpp index 1721e12..342acc8 100644 --- a/src/mp/util.cpp +++ b/src/mp/util.cpp @@ -134,7 +134,7 @@ std::tuple<ProcessId, SocketId> SpawnPr ocess(SpawnConnectInfoToArgsFn&& connect_ KJ_SYSCALL(fcntl(fds[0], F_SETFD, 0)); ProcessId pid = fork(); - if (pid == -1) { + if (pid == ProcessError) { throw std::system_error(errno, std::system_categor y(), "fork"); } // Parent process closes the descriptor for socket 0, child closes the

re: #274 (comment)

IMO it wouldn't make sense to a ProcessError constant because this constant would only be used in posix code where -1 is clearer. SocketError is different because it actually needs to be used by cross-platform code and I think the value is also slightly different in windows and posix implementations (unsigned vs signed).

enirox001 · 2026-07-08T15:54:58Z

            loop_lock->unlock();
            char buffer = 0;
-            KJ_SYSCALL(write(post_fd, &buffer, 1)); // NOLINT(bugprone-suspicious-semicolon)
+            loop->m_post_writer->write(&buffer, 1);


In 85e13a6 "proxy, refactor: Replace EventLoop wakeup fd integers with KJ stream objects"

Before this refactor, this method copied m_post_fd while holding m_mutex and then wrote to the local fd after unlocking.

Now it unlocks and then dereferences loop->m_post_writer. Is that safe if the event loop wakes up and tears down its streams while reset() is still executing?

Perhaps we could keep a local writer before unlocking?

re: #274 (comment)

Now it unlocks and then dereferences loop->m_post_writer. Is that safe if the event loop wakes up and tears down its streams while reset() is still executing?

This a good catch so added comments here and restored the previous pattern since the issues are a little subtle.

Dereferencing m_post_writer should be safe because the loop can't exit until the write happens. Added a comment describing this.

Dereferencing loop should also be safe because the m_post_writer member also won't be changed (when it is set to null) until after the write() call happens so that should also be safe.

But IIRC the previous pattern of copying members out of m_loop while holding the mutex was needed to prevent false positives from TSAN, which could complain about members being written after they were read without synchronization between, because it was not aware of the synchronization provided by socketpair reads and writes.

Now comments are added explaining most of this.

enirox001 · 2026-07-08T15:57:35Z

-    m_wait_fd = SocketError;
-    m_post_fd = SocketError;
+    m_wait_stream = nullptr;
+    m_post_stream = nullptr;


In 85e13a6 "proxy, refactor: Replace EventLoop wakeup fd integers with KJ stream objects"

Should m_post_writer also be reset during EventLoop teardown? It is initialized from m_post_stream, but the loop mehtod currently clears m_wait_stream and m_post_stream without clearing m_post_writer. I was thinking something along the lines of:

index c7b602e..e14455c 100644 --- a/src/mp/proxy.cpp +++ b/src/mp/proxy.cpp @@ -298,6 +298,7 @@ void EventLoop::loop() const Lock lock(m_mutex); m_wait_stream = nullptr; m_post_stream = nullptr; + m_post_writer = nullptr; m_async_fns.reset(); m_cv.notify_all(); }

re: #274 (comment)

Thanks, it does seem better to clear m_post_writer here for completeness, so added this

enirox001 · 2026-07-08T16:00:16Z

 //! errors in python unit tests.
 std::string LogEscape(const kj::StringTree& string, size_t max_size);

+using Stream = kj::Own<kj::AsyncIoStream>;


In 174604a "proxy, refactor: Change ConnectStream and ServeStream to accept stream objects"

Had a concern that the name Stream is fairly broad, so using it too widely could hide useful information at call sites. For example, inside EventLoop it seems helpful to see the concrete KJ type details directly, otherwise future code might start using mp::Stream everywhere just because the alias exists.

I agree with keeping it limited to external facing methods like ConnectStream and ServeStream, where the abstraction is useful, and avoiding it in internal code where the underlying type is more informative.

enirox001 · 2026-07-08T16:14:48Z

+        // Ignore ENOTCONN: on macOS/FreeBSD (unlike Linux), shutdown(SHUT_WR)
+        // returns ENOTCONN if the peer already closed the connection. This is
+        // expected when the destructor is triggered by a remote disconnect.
+        if (e.getType() != kj::Exception::Type::DISCONNECTED) throw;


In c5faf68 "proxy: Call shutdownWrite() in Connection destructor"

Is it a good idea to allow exceptions to escape from a destructor here?

Also, what happens if Connection wraps a stream that does not support shutdownWrite(), wouldn’t this try to perform an operation that isn't permitted and potentially terminate the process?

I think this should either only call shutdownWrite() for streams where it is valid, or catch/ignore the expected failure in the destructor

re: #274 (comment)

In c5faf68 "proxy: Call shutdownWrite() in Connection destructor"

Is it a good idea to allow exceptions to escape from a destructor here?

I think probably the answer to this is yes since the pattern is used consistently in capnproto code https://github.com/capnproto/capnproto/blob/v2/kjdoc/tour.md#exceptions-in-destructors and this seems better than alternatives of suppressing errors or terminating.

But you're right libmultiprocess isn't doing a great job dealing with these exceptions in many cases, and in this case noexcept(false) is needed here so have added that.

Also for defensiveness have added a check to handle the case where shutdownWrite() is not implemented, even though for all stream types we support (socketpairs, UNIX sockets, and TCP sockets) shutdownwrite and half-closes are supported

Add ProcessId = int type alias and apply it to WaitProcess, SpawnProcess (pid output argument), and callers. ProcessId type will be different on windows so this provides more portability.

Add SocketId = int and SocketError = -1 type aliases and apply SocketId to SpawnProcess (return type and callback parameter) and callers. SocketId type will be different on Windows, so this provides more portability. Co-authored-by: Sjors Provoost <sjors@sprovoost.nl>

Add SpawnConnectInfo type alias to pass socket handle from parent process to child process in more platform independent way.

gen.cpp used fork() directly via <unistd.h> to invoke the capnp compiler as a subprocess, but fork() is not available on Windows, so shouldn't be used in application code. Add a StartProcess(const std::vector<std::string>& args) function to util.h/util.cpp that spawns a process and returns its ProcessId, leaving the caller responsible for WaitProcess. On POSIX it uses posix_spawn; on Windows it can use CreateProcess. Update gen.cpp to replace the inline fork/exec/wait with mp::WaitProcess(mp::StartProcess(args)). There a change in behavior if starting the process fails, because KJ_FAIL_SYSCALL is now used for error reporting and the fs::weakly_canonical call is dropped (it is probably more useful to see literal executable name passed). Also previously, the child process could deadlock if exec() failed because threads in the parent may have held allocator or stdio mutex locks that are now leaked in the child. posix_spawn() avoids this entirely by never running arbitrary code in the child process. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>

Extract socket pair creation from SpawnProcess into a standalone SocketPair() function, and use it to replace the inline socketpair() call. This is pretty much a refactoring but technically there are two small behavior changes: - FD_CLOEXEC is now used to ensure sockets are not leaked if processes are spawned. This has no impact on SpawnProcess, but is better hygiene and could affect future callers of SocketPair(). - KJ_SYSCALL is now used to simplify error-handling. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>

…objects Replace the m_wait_fd/m_post_fd raw int members with m_wait_stream/m_post_stream kj::Own<kj::AsyncIoStream> and m_post_writer kj::Own<kj::OutputStream>. The constructor uses provider->newTwoWayPipe() instead of calling socketpair() directly. The loop() and post() methods write through m_post_writer instead of calling write() with a raw fd, and EventLoopRef::reset does the same.

kj::AsyncIoStream::getFd() was added in capnproto 0.9 (commit d27bfb8a4175b32b783de68d93dd1dbafadddea5, first released in 0.9.0). The code now uses getFd() in proxy.cpp, so 0.7 is no longer a sufficient minimum. Set olddeps version to 0.9.2, which is the patched 0.9.x release for CVE-2022-46149. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>

…m objects Instead of accepting raw file descriptor integers and wrapping them internally, ConnectStream and ServeStream now accept kj::Own<kj::AsyncIoStream> directly. This removes the assumption that the transport is always a local unix fd, making the API easier to adapt to other I/O types (e.g. Windows handles). The Stream type alias (kj::Own<kj::AsyncIoStream>) is added as a convenience. Callers are updated to wrap their fd with wrapSocketFd() before calling.

Flush pending Cap'n Proto release messages before closing the stream. When one side of a socket pair closes, the other side does not receive an onDisconnect event, so it relies on receiving release messages from the closing side to free its ProxyServer objects and shut down cleanly. Without this, Server objects are not freed by Cap'n Proto on disconnection. This also adds noexception(false) to the ~Connection destructor to start following the Cap'n Proto error handling pattern described in https://github.com/capnproto/capnproto/blob/v2/kjdoc/tour.md#exceptions-in-destructors Following the pattern should not be very important here because exceptions should never be leaked here, but in general this pattern should be preferable to alternative of suppressing errors entirely or terminating, instead of returning them to callers.

…ibraries On macOS, when libcapnp is built as a dynamic library and Bitcoin Core REDUCE_EXPORT option is used the RTTI typeinfo for kj::Exception has a different address in libcapnp.dylib versus the calling binary. This means catch (const kj::Exception& e) in the calling binary silently fails to match exceptions thrown by capnp, so the DISCONNECTED exception from shutdownWrite() propagates as a fatal uncaught exception instead of being suppressed as intended. This causes the Bitcoin Core macOS native CI job to fail with: Fatal uncaught kj::Exception: kj/async-io-unix.c++:491: disconnected: shutdown(fd, SHUT_WR): Socket is not connected The fix is to use kj::runCatchingExceptions/kj::throwRecoverableException, which use KJ's own thread-level exception interception mechanism rather than C++ RTTI-based matching, and therefore work correctly across dynamic library boundaries. This is the same approach used elsewhere in the codebase (proxy.cpp EventLoop::post, type-context.h server request handler) for the same reason. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>

MSVC error when building multiprocess.vcxproj: mp/util.h(146,46): error C2280: 'std::variant<T *,T>::variant(const std::variant<T *,T> &)': attempting to reference a deleted function [with T=mp::Lock] The PtrOrValue constructor used a ternary expression to initialize data: data(ptr ? ptr : std::variant<T*, T>{std::in_place_type<T>, args...}) Both arms are prvalues of type std::variant<T*,T>, so under C++17's mandatory copy elision no copy/move constructor should be invoked. GCC and Clang apply this correctly. MSVC does not apply guaranteed copy elision to ternary expressions in this context: it materializes the temporary and then attempts to copy-construct data from it. Since std::variant<Lock*,Lock> has a deleted copy constructor (Lock holds a std::unique_lock which is move-only), MSVC fails. Fix by initializing data to hold T*=ptr in the member initializer list, then emplacing T in-place in the constructor body if ptr is null. This avoids the ternary entirely and requires only the in-place constructor of T, not any variant copy or move. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>

MSVC warns (C4305, treated as error) about truncation from 'int' to 'const bool' when initializing static const bool members from integer bitwise-and expressions. Use constexpr bool with explicit != 0 to make the boolean conversion unambiguous. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>

MSVC does not correctly apply SFINAE when the substitution failure occurs in a default function argument that uses decltype of another function parameter (MSVC error C2039). Instead of silently excluding the overload, MSVC instantiates the function body and reports a hard error, e.g.: type-interface.h(62,56): error C2039: 'Calls': is not a member of 'capnp::List<mp::test::messages::Pair<capnp::Text,capnp::Text>, capnp::Kind::STRUCT>::Builder' The root cause is that MSVC evaluates default argument expressions outside the SFINAE immediate context when they reference function parameters via decltype. GCC and Clang treat this as a substitution failure and silently exclude the overload, as the standard intends. Replace all uses of the `* enable = nullptr` default-argument SFINAE pattern with C++20 requires clauses, which are well-supported on all three compilers and give cleaner constraint-violation diagnostics. Add two concepts to util.h to reduce repetition across the type-*.h headers: FieldTypeIs<T, U> - T's .get() returns exactly type U InterfaceField<T> - T's .get() returns a capnp interface type (a type that exposes a nested ::Calls member) Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>

This repo has introduced API changes to add Windows support to libmultiprocess (HANDLE-based IPC alongside the existing fd-based IPC). These changes require corresponding updates to Bitcoin Core, which are pending in bitcoin/bitcoin#35084. Until that PR merges, the Bitcoin Core CI jobs fail against master because Bitcoin Core has not yet been updated to use the new API. Switch the Bitcoin Core checkout in both jobs to use refs/pull/35084/merge so CI tests against the compatible version. A BITCOIN_CORE_REF env var is introduced at the top of the file; once (and keep the var in place for any future API compatibility cycles). Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>

On MSVC, std::terminate() does not print the exception message before calling abort()/fastfail, so exceptions thrown during mpgen execution appear as a bare 0xC0000409 exit code with no diagnostic output. Wrap main() in a try-catch to explicitly print the error to stderr and return 1 instead of crashing. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>

Bitcoin Core linter rejects it: https://github.com/bitcoin/bitcoin/actions/runs/24568789956/job/71835997334?pr=32387

ryanofsky

Thanks for the reviews! I dropped the cmake commits here since they require a different type of review and would make more sense to review separately.

Rebased 6cc729f -> 56220f8 (pr/wins.4 -> pr/wins.5, compare) due to conflicts, also applying review suggestions and dropping the cmake commits

Updated 56220f8 -> 491f1e8 (pr/wins.5 -> pr/wins.6, compare) fixing comments and commit organization

ryanofsky · 2026-07-10T18:58:32Z

-        }
-        _exit(1);
+    ProcessId pid;
+    if (int err = posix_spawn(&pid, argv[0], nullptr, nullptr, argv.data(), ::environ)) {


re: #274 (comment)

In ddd3945 util, refactor: Do not fork() and exec() separately: not posix_spawnp?

Good catch! Switched to posix_spawnp here (so PATH) is still used

ryanofsky · 2026-07-10T19:18:19Z


+//! Create a socket pair that can be used to communicate within a process or
+//! between parent and child processes.
+std::array<SocketId, 2> SocketPair();


re: #274 (comment)

In d9a5a68 util, refactor: Add SocketPair() and use it in SpawnProcess: it seems a bit odd to set FD_CLOEXEC and then unset it again for the child. The unset code also assume it was the only flag, since it sets 0. Maybe it's better to give this function a boolean argument for whether to set this flag on the child?

Good catch on assuming no other flags were set, this should be fixed now by calling F_GETFD. It still seems a little less error prone to explicitly unset FD_CLOEXEC right before forking, instead of adding an option and treating the socketpair fds differently

ryanofsky · 2026-07-10T19:20:17Z

re: #274 (comment)

Good catch, added SocketId here

ryanofsky · 2026-07-10T19:22:40Z

-        std::cerr << argv[1] << " is not a number or is larger than an int\n";
-        return 1;
-    }
+    mp::SocketId socket{mp::StartSpawned(argv[1])};


re: #274 (comment)

Didn't we lose our error handling here? std::stoi throws exceptions on bad input, so shouldn't we add a try-catch to prevent a crash?

Yes previous error handling provided more context. Changed to catch and throw a more descriptive exception. It can be up to callers whether to crash in this case (which probably indicates a bug not a runtime error) and crashing in the examples seems ok

ryanofsky · 2026-07-10T19:27:03Z

 //! errors in python unit tests.
 std::string LogEscape(const kj::StringTree& string, size_t max_size);

+using ProcessId = int;


re: #274 (comment)

IMO it wouldn't make sense to a ProcessError constant because this constant would only be used in posix code where -1 is clearer. SocketError is different because it actually needs to be used by cross-platform code and I think the value is also slightly different in windows and posix implementations (unsigned vs signed).

ryanofsky · 2026-07-10T19:29:03Z

            loop_lock->unlock();
            char buffer = 0;
-            KJ_SYSCALL(write(post_fd, &buffer, 1)); // NOLINT(bugprone-suspicious-semicolon)
+            loop->m_post_writer->write(&buffer, 1);


re: #274 (comment)

Now it unlocks and then dereferences loop->m_post_writer. Is that safe if the event loop wakes up and tears down its streams while reset() is still executing?

This a good catch so added comments here and restored the previous pattern since the issues are a little subtle.

Dereferencing m_post_writer should be safe because the loop can't exit until the write happens. Added a comment describing this.

Dereferencing loop should also be safe because the m_post_writer member also won't be changed (when it is set to null) until after the write() call happens so that should also be safe.

But IIRC the previous pattern of copying members out of m_loop while holding the mutex was needed to prevent false positives from TSAN, which could complain about members being written after they were read without synchronization between, because it was not aware of the synchronization provided by socketpair reads and writes.

Now comments are added explaining most of this.

ryanofsky · 2026-07-10T19:35:54Z

-    m_wait_fd = SocketError;
-    m_post_fd = SocketError;
+    m_wait_stream = nullptr;
+    m_post_stream = nullptr;


re: #274 (comment)

Thanks, it does seem better to clear m_post_writer here for completeness, so added this

ryanofsky · 2026-07-10T19:37:21Z

 //! errors in python unit tests.
 std::string LogEscape(const kj::StringTree& string, size_t max_size);

+using Stream = kj::Own<kj::AsyncIoStream>;


re: #274 (comment)

Agreed, glad the reasoning made sense!

ryanofsky · 2026-07-10T20:46:34Z

+        // Ignore ENOTCONN: on macOS/FreeBSD (unlike Linux), shutdown(SHUT_WR)
+        // returns ENOTCONN if the peer already closed the connection. This is
+        // expected when the destructor is triggered by a remote disconnect.
+        if (e.getType() != kj::Exception::Type::DISCONNECTED) throw;


re: #274 (comment)

In c5faf68 "proxy: Call shutdownWrite() in Connection destructor"

Is it a good idea to allow exceptions to escape from a destructor here?

I think probably the answer to this is yes since the pattern is used consistently in capnproto code https://github.com/capnproto/capnproto/blob/v2/kjdoc/tour.md#exceptions-in-destructors and this seems better than alternatives of suppressing errors or terminating.

But you're right libmultiprocess isn't doing a great job dealing with these exceptions in many cases, and in this case noexcept(false) is needed here so have added that.

Also for defensiveness have added a check to handle the case where shutdownWrite() is not implemented, even though for all stream types we support (socketpairs, UNIX sockets, and TCP sockets) shutdownwrite and half-closes are supported

ryanofsky mentioned this pull request Apr 23, 2026

Add windows support #231

Open

This was referenced Apr 23, 2026

ci: add Windows job #272

Draft

proxy: add local connection limit to ListenConnections #269

Merged

cmake: Increase cmake policy version #209

Open

Set cmake_minimum_required(VERSION 3.22) #175

Open

ViniciusCestarii reviewed May 8, 2026

View reviewed changes

Comment thread include/mp/util.h Outdated

ViniciusCestarii reviewed May 8, 2026

View reviewed changes

Comment thread src/mp/util.cpp Outdated

ViniciusCestarii reviewed May 8, 2026

View reviewed changes

Comment thread include/mp/util.h Outdated

DrahtBot mentioned this pull request May 12, 2026

build: prepare for subtree split #276

Closed

Sjors mentioned this pull request May 28, 2026

Mining interface tracking issue bitcoin/bitcoin#33777

Open

13 tasks

ryanofsky commented Jun 2, 2026

View reviewed changes

Comment thread include/mp/util.h Outdated

Comment thread src/mp/util.cpp Outdated

Comment thread include/mp/util.h Outdated

DrahtBot added the Needs rebase label Jun 9, 2026

Sjors mentioned this pull request Jun 17, 2026

Add nonunix platform support stratum-mining/sv2-tp#110

Draft

Sjors reviewed Jun 17, 2026

View reviewed changes

Comment thread include/mp/util.h

Comment thread include/mp/util.h

Sjors reviewed Jun 17, 2026

View reviewed changes

Comment thread include/mp/util.h

Sjors reviewed Jun 18, 2026

View reviewed changes

Comment thread include/mp/util.h

Comment thread include/mp/util.h

Comment thread src/mp/util.cpp

Comment thread src/mp/util.cpp Outdated

Sjors reviewed Jun 18, 2026

View reviewed changes

Sjors mentioned this pull request Jun 18, 2026

ipc: Add nonunix platform support bitcoin/bitcoin#35084

Open

ryanofsky force-pushed the pr/wins branch from 7cb83a5 to 68ed129 Compare June 22, 2026 18:13

ryanofsky commented Jun 22, 2026

View reviewed changes

DrahtBot removed the Needs rebase label Jun 22, 2026

ryanofsky force-pushed the pr/wins branch 2 times, most recently from a43e5a8 to 6cc729f Compare June 22, 2026 21:32

DrahtBot mentioned this pull request Jun 23, 2026

ci: Bump channel to nixos-26.05 #296

Open

Sjors reviewed Jun 23, 2026

View reviewed changes

This was referenced Jul 1, 2026

Server-wide ThreadPool #300

Draft

refactor: rename EventLoop::m_num_clients to m_num_refs #302

Merged

DrahtBot added the Needs rebase label Jul 5, 2026

enirox001 reviewed Jul 8, 2026

View reviewed changes

DrahtBot requested a review from enirox001 July 8, 2026 16:21

ryanofsky and others added 17 commits July 10, 2026 17:15

doc: Bump version 12 > 13

f2c4952

util, refactor: Add ProcessId type alias and use it

c67439a

Add ProcessId = int type alias and apply it to WaitProcess, SpawnProcess (pid output argument), and callers. ProcessId type will be different on windows so this provides more portability.

util, refactor: Add SpawnConnectInfo type alias and use it

3589a2a

Add SpawnConnectInfo type alias to pass socket handle from parent process to child process in more platform independent way.

doc: Remove trailing whitespace

491f1e8

Bitcoin Core linter rejects it: https://github.com/bitcoin/bitcoin/actions/runs/24568789956/job/71835997334?pr=32387

ryanofsky force-pushed the pr/wins branch from 6cc729f to 56220f8 Compare July 10, 2026 21:48

ryanofsky commented Jul 10, 2026

View reviewed changes

ryanofsky force-pushed the pr/wins branch from 56220f8 to 491f1e8 Compare July 10, 2026 23:18

DrahtBot removed the Needs rebase label Jul 10, 2026

Uh oh!

Conversation

ryanofsky commented Apr 22, 2026

Uh oh!

DrahtBot commented Apr 22, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Reviews

Conflicts

LLM Linter (✨ experimental)

Uh oh!

Uh oh!

Uh oh!

Uh oh!

ryanofsky left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Sjors commented Jun 17, 2026

Uh oh!

Sjors left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Sjors left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

ryanofsky left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Sjors left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

enirox001 left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

DrahtBot commented Apr 22, 2026 •

edited

Loading

ryanofsky left a comment •

edited

Loading

ryanofsky left a comment •

edited

Loading