Repository navigation
Conversation
|
The following sections might be updated with supplementary metadata relevant to reviewers and maintainers. ReviewsSee the guideline and AI policy for information on the review process.
If your review is incorrectly listed, please copy-paste ConflictsReviewers, this pull request conflicts with the following ones:
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:
Possible places where named args for integral literals may be used (e.g.
2026-10-20 07:26:50 |
|
We have miners using Stratum V2 that are on Windows and would like to see Windows IPC support landing, is there any estimate when they can expect this to materialize? We have a dozen of users who can help with testing if that's a blocker. |
|
@pavlenex testing would definitely be useful. I guess for that to work we'd need a stack of pull requests: this one here in libmultiprocess, one in Bitcoin Core that enables IPC support in the Windows Guix build, and then an SRI pull request that uses it. Testers would then have download (or build themselves) the custom bitcoin core and SRI binaries. I've added Windows support to my v32 wish list: bitcoin/bitcoin#33777 No guarantees obviously. |
|
I'll try to get this PR ready for review this week, split up into smaller commits and with ci passing. From there as Sjors mentioned there is a lot more work to do: more code changes in bitcoin/bitcoin#32387 that need to be made in bitcoin core, enabling IPC into windows builds in bitcoin core, enabling it in windows CI jobs with pycapnp, adding client support, probably adding a windows CI job to this repository. v32 sounds like a good target though and it is very useful to know there is demand for this feature, because it hasn't been a priority so far |
|
PR is split up into commits now and should be reviewable. CI is not passing but failures look like IWYU errors. I also opened bitcoin/bitcoin#35084 with corresponding bitcoin core changes. Windows support for bitcoin core can be tested with bitcoin/bitcoin#32387 which combines both PRs and enables IPC by default in windows builds. Rebased 2975fac -> cb16d2e ( Updated cb16d2e -> e563c96 ( Updated e563c96 -> d9fcac6 ( Added 1 commits d9fcac6 -> a1748e2 ( Updated a1748e2 -> 18fc188 ( Updated 18fc188 -> 7fd5ec4 ( |
e563c96 to
d9fcac6
Compare
|
How realistic is it to add a Windows CI job here? (can be cross-compiled) |
The lifecycle test repeatedly constructs and destroys TPTester to exercise IPC EventLoop teardown. On Windows it hangs intermittently in std::thread::join during teardown of libmultiprocess thread-local state. This is a known libmultiprocess Windows issue, not specific to sv2-tp: bitcoin-core/libmultiprocess#231 bitcoin/bitcoin#32387 The fix lives upstream and rewrites the EventLoop wakeup primitive (raw fd -> KJ stream) and adds shutdownWrite() in ~Connection. Until that lands and is backported into our libmultiprocess subtree, gate this particular test off on _WIN32. The other sv2 unit tests run normally on Windows; only the explicit teardown loop is affected. Assisted-by: GitHub Copilot Assisted-by: Anthropic Claude Opus 4
The template_provider_tests `client_tests`, `fee_timer_blocking_test` and `new_tip_bypasses_fee_timer_test` cases each construct and tear down a TPTester. Like the lifecycle test skipped in the previous commit, that teardown deadlocks intermittently on Windows in std::thread::join during libmultiprocess thread-local state cleanup. This is a known libmultiprocess Windows issue, not specific to sv2-tp: bitcoin-core/libmultiprocess#231 bitcoin/bitcoin#32387 Gate the three TPTester-using cases off on _WIN32 with a TODO. The non-IPC test `block_reserved_weight_floor` continues to run on Windows. Assisted-by: GitHub Copilot Assisted-by: Anthropic Claude Opus 4
Tearing down a TPTester (and with it the IPC EventLoop / per-thread state) intermittently deadlocks on Windows in std::thread::join during libmultiprocess thread-local cleanup. The fix lives upstream and rewrites the EventLoop wakeup primitive (raw fd -> KJ stream) and adds shutdownWrite() in ~Connection. Until that lands and is backported into our libmultiprocess subtree, paper over it in the test harness so the Windows CI job stays useful: - TPTesterHandle now heap-allocates and intentionally leaks the tester on Windows; the OS reclaims the remaining loop thread and IPC state at process exit. This only disables test cleanup, not the tests themselves. Non-Windows builds continue to own the tester by value. - sv2_tester_lifecycle_tests is gated off on _WIN32, since its whole purpose is to exercise repeated TPTester construction and destruction; leaking would defeat the test. - src/test/main.cpp installs a Windows-only Boost global fixture whose destructor runs at module teardown (after every test case has completed and Boost has tallied results). It flushes stdout/stderr and calls _exit() with 0 or 1 based on the Boost results, bypassing static destructors entirely so the leaked threads cannot fault and turn a green run into exit code 139. - lint-includes.py: allowlist boost/test/results_collector.hpp, required by the new fixture. References: bitcoin-core/libmultiprocess#231 bitcoin/bitcoin#32387 Assisted-by: GitHub Copilot Assisted-by: Anthropic Claude Opus 4 Assisted-by: Anthropic Claude Opus 4.7
enirox001
left a comment
enirox001
left a comment
There was a problem hiding this comment.
Code Review 7861351
Light tACK tested natively on Windows 11 arm64 under Parallels using Visual Studio 2026/msvc 19.51 and capnp from vcpkg.
The use of non unix alternatives to enable Windows support here is nice, and the implementation seems good overall. I’m not very familiar with Windows code, but I left a few nits and suggestions.
mptest did fail to compile with msvc error C1128 number of sections exceeded object file format limit unless /bigobj is enabled.
Adding /bigobj for the mptest target allowed the build and tests to complete successfully.
Perhaps we could add the following after mptest is created in test/CMakeLists.txt?
target_compile_options(mptest PRIVATE
$<$<CXX_COMPILER_ID:MSVC>:/bigobj>
)
| std::array<SocketId, 2> SocketPair(); | ||
|
|
||
| //! Close a socket, throwing a KJ exception on failure. | ||
| void CloseSocket(SocketId fd); |
There was a problem hiding this comment.
In commit 15aa914: util, test: Add CloseSocket, use SocketId
I think this should be used here as well
index e8d1533..86616ce 100644
--- a/test/mp/test/spawn_tests.cpp
+++ b/test/mp/test/spawn_tests.cpp
@@ -141,7 +141,7 @@ KJ_TEST("SpawnProcess does not run callback in child")
std::lock_guard<std::mutex> g(target_mutex);
return {"true", std::move(connect_info)};
})};
- ::close(socket);
+ CloseSocket(socket);
int status{0};
// Give the child some time to exit. If it does not, terminate it and| std::array<SocketId, 2> SocketPair(); | ||
|
|
||
| //! Close a socket, throwing a KJ exception on failure. | ||
| void CloseSocket(SocketId fd); |
There was a problem hiding this comment.
In commit 15aa914: util, test: Add CloseSocket, use SocketId
nit: Since we are not really using fd, so I think the parameter name should be changed to socket as well?
| } | ||
|
|
||
| UnixListener listener; | ||
| SocketListener listener; |
There was a problem hiding this comment.
In commit c79b49a: util, test: Replace UnixListener with SocketListener
There is a stale comment still referencing UnixListener in the ListenSetup class, should be changed
index a90294a..7d1298f 100644
--- a/test/mp/test/listen_tests.cpp
+++ b/test/mp/test/listen_tests.cpp
@@ -64,7 +64,7 @@ public:
};
//! Runs a server EventLoop on its own thread, starts ListenConnections() on a
-//! UnixListener socket, and records connection/disconnection counts through
+//! SocketListener socket, and records connection/disconnection counts through
//! EventLoop test hooks
class ListenSetup
{|
|
||
| KJ_TEST("SpawnProcess does not run callback in child") | ||
| { | ||
| // This test is designed to fail deterministically if fd_to_args is invoked |
There was a problem hiding this comment.
In commit 860e2ff: util, test: guard Unix-only code for Windows build
nit: I see fd_to_args here, I believe we are now using spawn_argv
| namespace test { | ||
| namespace { | ||
|
|
||
| constexpr auto FAILURE_TIMEOUT = std::chrono::seconds{30}; |
There was a problem hiding this comment.
In 860e2ff: util, test: guard Unix-only code for Windows build
This is used in the ifndef WIN32 guard block below, but it is defined outside, would this be a problem? should we perhaps move or duplicate the definition into the guard below?
index e8d1533..3498efc 100644
--- a/test/mp/test/spawn_tests.cpp
+++ b/test/mp/test/spawn_tests.cpp
@@ -40,6 +40,7 @@ constexpr auto FAILURE_TIMEOUT = std::chrono::seconds{30};
#ifdef WIN32
KJ_TEST("SpawnProcess does not hang if child never connects to named pipe")
{
+ constexpr auto FAILURE_TIMEOUT = std::chrono::seconds{30};
// Without FILE_FLAG_OVERLAPPED on the named pipe, ConnectNamedPipe blocks
// forever if the child exits without opening the pipe. Verify SpawnProcess
// detects child exit and throws instead of hanging.| namespace mp { | ||
| namespace { | ||
|
|
||
| #ifndef WIN32 |
There was a problem hiding this comment.
In commit 860e2ff: util, test: guard Unix-only code for Windows build
nit: This guards the definitions for the helpers, but does not guard the calls, in later commits this is rectified as the windows implementations are introduced, but the commit message part that says so the file compiles on Windows without modification should perhaps be rectified
| #include <kj/common.h> | ||
| #include <kj/debug.h> | ||
| #include <kj/test.h> | ||
| #include <string> |
There was a problem hiding this comment.
In commit ee09c2b: util: Add Windows CommandLineFromArgv escaping function
I think the strings and vector includes can be removed from the WIN32 guard, since they are already included above?
index ea8f2e7..5ae0ebb 100644
--- a/test/mp/test/util_tests.cpp
+++ b/test/mp/test/util_tests.cpp
@@ -12,8 +12,6 @@
#ifdef WIN32
#include <shellapi.h>
-#include <string>
-#include <vector>
#endif
namespace mp {| @@ -309,10 +318,11 @@ std::string CommandLineFromArgv(const std::vector<std::string>& argv) | |||
| std::tuple<ProcessId, SocketId> SpawnProcess(const std::function<std::vector<std::string>(std::string)>& spawn_argv) | |||
There was a problem hiding this comment.
In commit 7861351: util: Add Windows support
I am a bit concerned that the SpawnProcess might be able to leak if something goes wrong between the many initialisation steps or during CreateProcess? Could we have a way to defer cleanup here perhaps?
Rebase bitcoin-core/libmultiprocess#231 onto upstream master 2dba130478 and include the MinGW ThreadContext workaround from bitcoin/bitcoin#32387. Preserve current upstream race fixes and CI. The rebased upstream history is retained locally on codex/libmultiprocess-windows.
Rebase bitcoin-core/libmultiprocess#231 onto upstream master 2dba130478 and include the MinGW ThreadContext workaround from bitcoin/bitcoin#32387. Preserve current upstream race fixes and CI. The rebased upstream history is retained locally on codex/libmultiprocess-windows.
…g to named pipe SpawnProcess on Windows creates a named pipe with PIPE_WAIT and then calls ConnectNamedPipe synchronously. If the child exits or crashes before connecting, ConnectNamedPipe blocks forever with no recovery path. Fix by opening the pipe with FILE_FLAG_OVERLAPPED and using WaitForMultipleObjects on both the connect event and the child process handle. If the process handle signals first, the child died without connecting and SpawnProcess throws instead of hanging. Since the pipe is now in overlapped mode, WriteFile also requires an OVERLAPPED structure; use GetOverlappedResult with bWait=TRUE to handle both synchronous and asynchronous completion. Add a Windows-only test that spawns a child which exits immediately without opening the named pipe and asserts SpawnProcess does not block. (bitcoin-core/libmultiprocess#231 (comment)) Co-Authored-By: ViniciusCestarii <viniciuscestari01@gmail.com> This change was written with Claude Sonnet 4.6.
…g to named pipe SpawnProcess on Windows creates a named pipe with PIPE_WAIT and then calls ConnectNamedPipe synchronously. If the child exits or crashes before connecting, ConnectNamedPipe blocks forever with no recovery path. Fix by opening the pipe with FILE_FLAG_OVERLAPPED and using WaitForMultipleObjects on both the connect event and the child process handle. If the process handle signals first, the child died without connecting and SpawnProcess throws instead of hanging. Since the pipe is now in overlapped mode, WriteFile also requires an OVERLAPPED structure; use GetOverlappedResult with bWait=TRUE to handle both synchronous and asynchronous completion. Add a Windows-only test that spawns a child which exits immediately without opening the named pipe and asserts SpawnProcess does not block. (bitcoin-core#231 (comment)) Co-Authored-By: ViniciusCestarii <viniciuscestari01@gmail.com> This change was written with Claude Sonnet 4.6.
…g to named pipe SpawnProcess on Windows creates a named pipe with PIPE_WAIT and then calls ConnectNamedPipe synchronously. If the child exits or crashes before connecting, ConnectNamedPipe blocks forever with no recovery path. Fix by opening the pipe with FILE_FLAG_OVERLAPPED and using WaitForMultipleObjects on both the connect event and the child process handle. If the process handle signals first, the child died without connecting and SpawnProcess throws instead of hanging. Since the pipe is now in overlapped mode, WriteFile also requires an OVERLAPPED structure; use GetOverlappedResult with bWait=TRUE to handle both synchronous and asynchronous completion. Add a Windows-only test that spawns a child which exits immediately without opening the named pipe and asserts SpawnProcess does not block. (bitcoin-core/libmultiprocess#231 (comment)) Co-Authored-By: ViniciusCestarii <viniciuscestari01@gmail.com> This change was written with Claude Sonnet 4.6.
…g to named pipe SpawnProcess on Windows creates a named pipe with PIPE_WAIT and then calls ConnectNamedPipe synchronously. If the child exits or crashes before connecting, ConnectNamedPipe blocks forever with no recovery path. Fix by opening the pipe with FILE_FLAG_OVERLAPPED and using WaitForMultipleObjects on both the connect event and the child process handle. If the process handle signals first, the child died without connecting and SpawnProcess throws instead of hanging. Since the pipe is now in overlapped mode, WriteFile also requires an OVERLAPPED structure; use GetOverlappedResult with bWait=TRUE to handle both synchronous and asynchronous completion. Add a Windows-only test that spawns a child which exits immediately without opening the named pipe and asserts SpawnProcess does not block. (bitcoin-core#231 (comment)) Co-Authored-By: ViniciusCestarii <viniciuscestari01@gmail.com> This change was written with Claude Sonnet 4.6.
…g to named pipe SpawnProcess on Windows creates a named pipe with PIPE_WAIT and then calls ConnectNamedPipe synchronously. If the child exits or crashes before connecting, ConnectNamedPipe blocks forever with no recovery path. Fix by opening the pipe with FILE_FLAG_OVERLAPPED and using WaitForMultipleObjects on both the connect event and the child process handle. If the process handle signals first, the child died without connecting and SpawnProcess throws instead of hanging. Since the pipe is now in overlapped mode, WriteFile also requires an OVERLAPPED structure; use GetOverlappedResult with bWait=TRUE to handle both synchronous and asynchronous completion. Add a Windows-only test that spawns a child which exits immediately without opening the named pipe and asserts SpawnProcess does not block. (bitcoin-core/libmultiprocess#231 (comment)) Co-Authored-By: ViniciusCestarii <viniciuscestari01@gmail.com> This change was written with Claude Sonnet 4.6.
Remove recently introduced SpawnConnectInfo and SpawnConnectInfoToArgsFn type aliases since they are the same on all platforms and might obscure the fact that connect info should be treated as an opaque string. Co-authored-by: Sjors Provoost <sjors@sprovoost.nl>
This change was written with Claude Sonnet 4.6.
Replace the Unix-only UnixListener test helper with a cross-platform SocketListener backed by a std::variant<sockaddr_un> (TCP arm will be added later for Wine compatibility). - test/socketlistener.h: New header with SocketListener class extracted from the pattern of UnixListener but using SocketId/SocketError types, std::filesystem for temp dir cleanup, and mp::CloseSocket for teardown. Uses std::variant<sockaddr_un> so the TCP arm can be appended later without restructuring the class. - test/unixlistener.h: Deleted. - test/listen_tests.cpp: Switch to socketlistener.h and socketlistener. - test/connect_tests.cpp: Switch to socketlistener.h and socketlistener. This change was written with Claude Sonnet 4.6.
Move POSIX-only headers in util.cpp into a #ifndef WIN32 block so the file compiles on Windows without modification. Guard the Unix-only helpers (MakeArgv, MaxFd, ChildFail, SpawnError*, ReadSpawnResult, WriteSpawnError, KillAndReapChild) with a single #ifndef WIN32 block. Guard the extern "C" environ declaration likewise. Guard Unix-only includes in spawn_tests.cpp and connect_tests.cpp and wrap the Unix-only spawn tests with #ifndef WIN32. No Windows implementations are added here; those come in a later commit. This change was written with Claude Sonnet 4.6.
Co-authored-by: Sjors Provoost <sjors@sprovoost.nl>
Add Windows-specific code to support building and running on Windows: - util.h: Guard ProcessId/SocketId/SocketError type aliases with WIN32 ifdefs so they use SOCKET/uintptr_t on Windows and int on Unix. Add winsock2.h include on Windows. - util.cpp: Guard Unix-specific system headers with WIN32 ifdefs. Add Windows-specific includes (windows.h, winsock2.h). Guard MaxFd() with #ifndef WIN32. Add GetCurrentThreadId() branch in ThreadName(). Add win32Socketpair() forward-declare. Add Windows branch in SocketPair() using win32Socketpair(). Add CommandLineFromArgv() helper needed to construct CreateProcess command lines. Add Windows branch in SpawnProcess() using named pipes and WSADuplicateSocket to pass socket to child. Add Windows branch in StartSpawned() reading socket from named pipe. Add Windows branch in WaitProcess() using WaitForSingleObject/GetExitCodeProcess. - proxy.cpp: Add SocketOutputStream class on Windows (analogous to FdOutputStream but using SOCKET/send()). Add Windows branch in EventLoop constructor to create m_post_writer using SocketOutputStream. Co-Authored-By: ViniciusCestarii <viniciuscestari01@gmail.com>
…g to named pipe SpawnProcess on Windows creates a named pipe with PIPE_WAIT and then calls ConnectNamedPipe synchronously. If the child exits or crashes before connecting, ConnectNamedPipe blocks forever with no recovery path. Fix by opening the pipe with FILE_FLAG_OVERLAPPED and using WaitForMultipleObjects on both the connect event and the child process handle. If the process handle signals first, the child died without connecting and SpawnProcess throws instead of hanging. Since the pipe is now in overlapped mode, WriteFile also requires an OVERLAPPED structure; use GetOverlappedResult with bWait=TRUE to handle both synchronous and asynchronous completion. Add a Windows-only test that spawns a child which exits immediately without opening the named pipe and asserts SpawnProcess does not block. (bitcoin-core#231 (comment)) Co-Authored-By: ViniciusCestarii <viniciuscestari01@gmail.com> This change was written with Claude Sonnet 4.6.
Allow code to compile without pthreads available, as required for MSVC compatibility. Avoid unconditional POSIX calls (fork, posix_spawn, pthread_getname_np) by moving them into #ifndef WIN32 or HAVE_PTHREAD_* guards. When pthreads is available on Windows (detected via cmake HAVE_PTHREAD_* checks), still use it for thread name reporting since it provides useful information at low cost. Also add Threads::Threads as an explicit dependency of the multiprocess library. proxy.cpp directly uses thread_local, std::this_thread, and std::thread, and the dependency was previously satisfied only through transitive linkage from CapnProto::kj-async. This change was written with Claude Sonnet 4.6.
Add TCP (sockaddr_in) support to SocketListener and platform-conditional header guards so the class compiles and works on Windows: - Wrap POSIX headers (arpa/inet.h, netinet/in.h, sys/socket.h, sys/un.h) in #else of #ifdef WIN32, with Windows equivalents (afunix.h, ws2tcpip.h) in the #ifdef WIN32 branch. - Add a WsaInit static initializer on Windows that calls WSAStartup before any socket operations. Tests create sockets directly (not via StartSpawned), so Winsock must be initialized here. - Add Init(sockaddr_in&) and Connect(const sockaddr_in&) overloads for TCP loopback connections. - Use TCP on Windows to work around Wine's lack of the AcceptEx extension required by KJ's AF_UNIX listener; Unix platforms continue using AF_UNIX. - Expand std::variant<sockaddr_un> to std::variant<sockaddr_in, sockaddr_un>. This change was written with Claude Sonnet 4.6.
- shell.nix: add `windows` parameter that selects pkgs.pkgsCross.mingwW64
as the cross target; also change crossPkgs default from import<nixpkgs>{}
to null (cleaner API). When windows=true, add native pkgs.capnproto to
nativeBuildInputs so capnp/capnpc-c++ are in PATH for cmake code generation,
and add wine64Packages.staging so ctest can run mptest.exe via wine.
Change llvmBase to always use pkgs (native) instead of crossPkgs.
- ci/configs/windows.bash: new config that cross-compiles with mingw,
sets CMAKE_SYSTEM_NAME=Windows, CMAKE_FIND_ROOT_PATH_MODE_PROGRAM=NEVER
(so cmake finds native capnp from PATH), CMAKE_CROSSCOMPILING_EMULATOR=wine
(so ctest runs mptest.exe via wine), and sets MPGEN_PRE_BUILD=1.
- ci/scripts/ci.sh: add MPGEN_PRE_BUILD support: when set, build native mpgen
in $CI_DIR-native before the main cross build, then inject
-DMPGEN_EXECUTABLE into CMAKE_ARGS. This is needed because cmake's
add_custom_command does not use CMAKE_CROSSCOMPILING_EMULATOR, so the
cross-compiled mpgen.exe cannot be used as a code generator directly.
This change was written with Claude Sonnet 4.6.
The proxy-server code generated for test/mp/test/foo.capnp now has more sections than MSVC's default object file format allows, failing the Windows native VS CI job with "error C1128: number of sections exceeded object file format limit: compile with /bigobj" (https://github.com/bitcoin/bitcoin/actions/runs/36945085020/job/110645241551). Bitcoin Core already passes /bigobj for its own debug builds. Not reproduced locally; based on the error message. This change was written with Claude Opus 5.5 (1M context).
|
Can you give this a rebase? I'll push an update to #272 after that. |
|
Rebased de40396 -> a8e8c55 ( |
7 participants
Add support for running on windows. These changes make the libmultiprocess API more generic, using stream types instead of file descriptors. All features are supported, including spawning processes with socket connections to the parent process. These changes were originally made in bitcoin/bitcoin#32387