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.
<!--e57a25ab6845829454e8d69fc972939a-->
The following sections might be updated with supplementary metadata relevant to reviewers and maintainers.
<!--021abf342d371248e50ceaed478a90ca-->
See the guideline and AI policy for information on the review process.
If your review is incorrectly listed, please copy-paste <code><!--meta-tag:bot-skip--></code> into the comment that the bot should ignore.
<!--174a7506f384e20aa4161008e828411d-->
Reviewers, 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.
<!--5faf32d7da4f0f540f40219e4f7537a3-->
273 | @@ -271,6 +274,10 @@ using ConnectInfoToArgsFn = std::function<std::vector<std::string>(const Connect 274 | //! it returns. Returns child process id and socket id. 275 | std::tuple<ProcessId, SocketId> SpawnProcess(ConnectInfoToArgsFn&& connect_info_to_args); 276 | 277 | +//! Spawn a process and return its process id. Caller should call WaitProcess 278 | +//! on the returned id. 279 | +ProcessId SpawnProcess(const std::vector<std::string>& args);
Is this declaration proposital? It has no definition anywhere
re: #274 (review)
In commit "util, refactor: Fix PtrOrValue constructor for move-only types on MSVC" (1060a95de217025f4ea0a7f56a3936aec57834c1)
Is this declaration proposital? It has no definition anywhere
Good catch. No this declaration is just a mistake accidentally to this commit. Will remove
In 1060a95de217025f4ea0a7f56a3936aec57834c1 util, refactor: Fix PtrOrValue constructor for move-only types on MSVC: this seems unrelated?
re: #274 (review)
Removed now
re: #274 (review)
In 1060a95 util, refactor: Fix PtrOrValue constructor for move-only types on MSVC: this seems unrelated?
Thanks, dropped this
176 | @@ -176,16 +177,20 @@ SocketId StartSpawned(const ConnectInfo& connect_info) 177 | return std::stoi(connect_info); 178 | } 179 | 180 | -void ExecProcess(const std::vector<std::string>& args) 181 | +ProcessId ExecProcess(const std::vector<std::string>& args)
Now that this function does fork() + execvp() rather than just execvp, is ExecProcess still the intended name?
re: #274 (review)
In commit "util, refactor: Handle forking inside ExecProcess" (b16f8c4b47ba80088a26e62c5251d0a88b0d45cb)
Now that this function does fork() + execvp() rather than just execvp, is ExecProcess still the intended name?
Good point, I don't think an ExecProcess is a bad name but it could be misleading, and something like RunProcess could be better.
Looking at this again, it was a pretty minor nit. ExecProcess is a fine name
re: #274 (review)
Looking at this again, it was a pretty minor nit. ExecProcess is a fine name
Wound up renaming to StartProcess, which I think should be an improvement
272 | +//! pair. Calls connect_info_to_args callback with a connection string that 273 | +//! needs to be passed to the child process, and executes the argv command line 274 | +//! it returns. Returns child process id and socket id. 275 | +std::tuple<ProcessId, SocketId> SpawnProcess(ConnectInfoToArgsFn&& connect_info_to_args); 276 | + 277 | +//! Initialize spawned child process using the ConnectInfo string passed to it,
StartSpawned reads as imperative, but the body just parses an int out of the connect-info string (and the header comment seems to describe more work than the function actually does). Would something like ParseConnectInfo fit better?
I might be missing the full picture here.
Yes, I was missing the full picture. Just looked PR 231
re: #274 (review)
the header comment seems to describe more work than the function actually does
Yes, you linked to the relevant change, but just to answer the question, while on unix this only needs to parse the descriptor number, on windows it needs to do more and read a socket descriptor over a pipe.
Thanks for the review! Will update with suggestions.
I opened https://github.com/stratum-mining/sv2-tp/pull/110 to test the changes in the Template Provider.
248 | @@ -249,6 +249,8 @@ std::string ThreadName(const char* exe_name); 249 | //! errors in python unit tests. 250 | std::string LogEscape(const kj::StringTree& string, size_t max_size); 251 | 252 | +using ProcessId = int;
In 36c91a0c73d5fc5ac616c43ed2a0fdfdea115b81 util, refactor: Add ProcessId type alias and use it: the commit message could mention that Windows uses a different underlying type.
re: #274 (review)
In 36c91a0 util, refactor: Add ProcessId type alias and use it: the commit message could mention that Windows uses a different underlying type.
Thanks, added this to commit message
249 | @@ -250,9 +250,11 @@ std::string ThreadName(const char* exe_name); 250 | std::string LogEscape(const kj::StringTree& string, size_t max_size); 251 | 252 | using ProcessId = int; 253 | +using SocketId = int; 254 | +constexpr SocketId SocketError{-1};
In 94af41bb55b773787ad1cf195c3b65a68568a1f6 util, refactor: Add SocketId type alias and use it: SocketError is unused in this commit. Maybe do:
diff --git a/include/mp/proxy-io.h b/include/mp/proxy-io.h
index d7b9f0e546..cae4ac08e9 100644
--- a/include/mp/proxy-io.h
+++ b/include/mp/proxy-io.h
@@ -310,8 +310,8 @@ public:
//! Pipe read handle used to wake up the event loop thread.
- int m_wait_fd = -1;
+ SocketId m_wait_fd = SocketError;
//! Pipe write handle used to wake up the event loop thread.
- int m_post_fd = -1;
+ SocketId m_post_fd = SocketError;
//! Number of clients holding references to ProxyServerBase objects that
diff --git a/src/mp/proxy.cpp b/src/mp/proxy.cpp
index eb0e841291..4783e1f110 100644
--- a/src/mp/proxy.cpp
+++ b/src/mp/proxy.cpp
@@ -217,6 +217,6 @@ EventLoop::~EventLoop()
KJ_ASSERT(m_post_fn == nullptr);
KJ_ASSERT(!m_async_fns);
- KJ_ASSERT(m_wait_fd == -1);
- KJ_ASSERT(m_post_fd == -1);
+ KJ_ASSERT(m_wait_fd == SocketError);
+ KJ_ASSERT(m_post_fd == SocketError);
KJ_ASSERT(m_num_clients == 0);
@@ -269,6 +269,6 @@ void EventLoop::loop()
KJ_SYSCALL(::close(post_fd));
const Lock lock(m_mutex);
- m_wait_fd = -1;
- m_post_fd = -1;
+ m_wait_fd = SocketError;
+ m_post_fd = SocketError;
m_async_fns.reset();
m_cv.notify_all();
Although IIUC this will go away in a later commit.
re: #274 (review)
In 94af41b util, refactor: Add SocketId type alias and use it:
SocketErroris unused in this commit. Maybe do:
Thanks, applied patch!
Although IIUC this will go away in a later commit.
Also true, but seems nice to be complete here
Studied two first three commits...
252 | @@ -253,17 +253,22 @@ using ProcessId = int; 253 | using SocketId = int; 254 | constexpr SocketId SocketError{-1}; 255 | 256 | +//! Information about parent process passed to child process as a command-line 257 | +//! argument. On unix this is the child socket fd number formatted as a string.
In beaa50a046190277a91cdf9ca87c169fd2516c55 util, refactor: Add ConnectInfo type alias and use it: maybe call it SpawnConnectInfo?
re: #274 (review)
In beaa50a util, refactor: Add ConnectInfo type alias and use it: maybe call it
SpawnConnectInfo?
Makes sense, renamed
269 | @@ -270,10 +270,9 @@ std::tuple<ProcessId, SocketId> SpawnProcess(ConnectInfoToArgsFn&& connect_info_ 270 | //! returning a socket id for communicating with the parent process. 271 | SocketId StartSpawned(const ConnectInfo& connect_info); 272 | 273 | -//! Call execvp with vector args. 274 | -//! Not safe to call in a post-fork child of a multi-threaded process.
In b16f8c4b47ba80088a26e62c5251d0a88b0d45cb util, refactor: Handle forking inside ExecProcess: I think you still need some sort of warning because the execvp failed branch in the child is not async-signal-safe.
re: #274 (review)
In b16f8c4 util, refactor: Handle forking inside ExecProcess: I think you still need some sort of warning because the
execvp failedbranch in the child is not async-signal-safe.
Agree there is an issue here, although the warning doesn't really help because it is telling the caller to do something it has no control over, and the function itself is breaking the safety rule internally. Fixed by switching to posix_spawn.
269 | @@ -270,10 +270,9 @@ std::tuple<ProcessId, SocketId> SpawnProcess(ConnectInfoToArgsFn&& connect_info_ 270 | //! returning a socket id for communicating with the parent process. 271 | SocketId StartSpawned(const ConnectInfo& connect_info); 272 | 273 | -//! Call execvp with vector args. 274 | -//! Not safe to call in a post-fork child of a multi-threaded process. 275 | -//! Currently only used by mpgen at build time.
In b16f8c4 util, refactor: Handle forking inside ExecProcess: this comment shouldn't be dropped, as it's a warning against using this in run time without dealing get signal-safety issue above first.
re: #274 (review)
In b16f8c4 util, refactor: Handle forking inside ExecProcess: this comment shouldn't be dropped, as it's a warning against using this in run time without dealing get signal-safety issue above first.
Note this review comment seems to be on same lines of code as #274 (review), so I think this is a duplicate suggestion
118 | @@ -119,10 +119,7 @@ std::string LogEscape(const kj::StringTree& string, size_t max_size) 119 | 120 | std::tuple<ProcessId, SocketId> SpawnProcess(ConnectInfoToArgsFn&& connect_info_to_args) 121 | { 122 | - SocketId fds[2]; 123 | - if (socketpair(AF_UNIX, SOCK_STREAM, 0, fds) != 0) { 124 | - throw std::system_error(errno, std::system_category(), "socketpair");
In 022b29b776759a246296cad578fbde97d47149a3 util, refactor: Add SocketPair() and use it in SpawnProcess: changing throw std::system_error and != 0 check, to KJ_SYSCALL seems fine, but still a slight behavior change?
re: #274 (review)
In 022b29b util, refactor: Add SocketPair() and use it in SpawnProcess: changing
throw std::system_errorand!= 0check, toKJ_SYSCALLseems fine, but still a slight behavior change?
Thanks, noted in commit message
158 | @@ -158,6 +159,16 @@ std::tuple<ProcessId, SocketId> SpawnProcess(ConnectInfoToArgsFn&& connect_info_ 159 | } 160 | } 161 | 162 | + // Explicitly clear FD_CLOEXEC on the child's socket before calling 163 | + // exec, so the fd survives into the spawned process regardless of how 164 | + // the socket was created. 165 | + int flags = fcntl(fds[0], F_GETFD); 166 | + if (flags == -1) throw std::system_error(errno, std::system_category(), "fcntl F_GETFD");
In 24c5e57fddec43c3e46fff36f93fe5f9f35d05bd util: Clear FD_CLOEXEC on child socket before exec: to not make things more non-fork-safe:
diff --git a/src/mp/util.cpp b/src/mp/util.cpp
index 15400215e4..b2a545ef5f 100644
--- a/src/mp/util.cpp
+++ b/src/mp/util.cpp
@@ -62,4 +62,12 @@ size_t MaxFd()
}
+template <size_t N>
+[[noreturn]] void ChildProcessError(const char (&msg)[N])
+{
+ const ssize_t writeResult = ::write(STDERR_FILENO, msg, N - 1);
+ (void)writeResult;
+ _exit(126);
+}
+
} // namespace
@@ -144,8 +152,5 @@ std::tuple<ProcessId, SocketId> SpawnProcess(ConnectInfoToArgsFn&& connect_info_
throw std::system_error(errno, std::system_category(), "close");
}
- static constexpr char msg[] = "SpawnProcess(child): close(fds[1]) failed\n";
- const ssize_t writeResult = ::write(STDERR_FILENO, msg, sizeof(msg) - 1);
- (void)writeResult;
- _exit(126);
+ ChildProcessError("SpawnProcess(child): close(fds[1]) failed\n");
}
@@ -164,8 +169,12 @@ std::tuple<ProcessId, SocketId> SpawnProcess(ConnectInfoToArgsFn&& connect_info_
// the socket was created.
int flags = fcntl(fds[0], F_GETFD);
- if (flags == -1) throw std::system_error(errno, std::system_category(), "fcntl F_GETFD");
+ if (flags == -1) {
+ ChildProcessError("SpawnProcess(child): fcntl(fds[0], F_GETFD) failed\n");
+ }
if (flags & FD_CLOEXEC) {
flags &= ~FD_CLOEXEC;
- if (fcntl(fds[0], F_SETFD, flags) == -1) throw std::system_error(errno, std::system_category(), "fcntl F_SETFD");
+ if (fcntl(fds[0], F_SETFD, flags) == -1) {
+ ChildProcessError("SpawnProcess(child): fcntl(fds[0], F_SETFD) failed\n");
+ }
}
Was there something in particular that led you this (hardening) commit?
re: #274 (review)
In 24c5e57 util: Clear FD_CLOEXEC on child socket before exec: to not make things more non-fork-safe:
Good catch. An easier to way to make it safe was just to move the CLOEXEC code before the fork() call so now doing that
re: #274 (review)
Was there something in particular that led you this (hardening) commit?
I don't remember exactly. It was definitely a bug of some kind. I think an earlier version of the code was using newTwoWayPipe which wraps a socket pair and sets CLOEXEC. Also originally SpawnProcess required callers to construct the socket pair, taking the child socket as an argument instead of both sockets. But this made calling code more complicated, so now SpawnProcess returns a parent socket instead of accepting a child socket.
Now since SpawnProcess is creating the socket pair it knows whether CLOEXEC is set so this code is not necessary. But I think it's good to use CLOEXEC to prevent accidental leaks so I did keep some of this and integrated it better with the earlier commit introducing SocketPair.
251 | @@ -250,6 +252,8 @@ std::string ThreadName(const char* exe_name); 252 | //! errors in python unit tests. 253 | std::string LogEscape(const kj::StringTree& string, size_t max_size); 254 | 255 | +using Stream = kj::Own<kj::AsyncIoStream>;
In 091f5e16dcd9f08d92aafbae4feee03566d0041e proxy, refactor: Change ConnectStream and ServeStream to accept stream objects: would it make sense to introduce this earlier, in 3c81cf27ead7add93c69a86ffb97bb51b9fe799a?
re: #274 (review)
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 (review)
Agreed, glad the reasoning made sense!
210 | @@ -211,6 +211,18 @@ class Logger 211 | 212 | std::string LongThreadName(const char* exe_name); 213 | 214 | +inline SocketId StreamSocketId(const Stream& stream)
In 7cb83a5d53476585c12dccabf84cc56294f64c5e cmake: Fix CapnProto tool paths broken by Ubuntu Noble packaging bug: StreamSocketId is unused and stays that way? Given that it's potentially unsafe to throw, maybe it's better to drop this?
re: #274 (review)
In 7cb83a5 cmake: Fix CapnProto tool paths broken by Ubuntu Noble packaging bug:
StreamSocketIdis unused and stays that way? Given that it's potentially unsafe to throw, maybe it's better to drop this?
Now removed. I think this was used previously when code was using newTwoWayPipe to create streams and using StreamSocketId to turn them into socket ids. Now it does the opposite calling SocketPair starting from socket ids, and calling MakeStream to turn them into streams.
Probably will not go back to the previous approach since it made SpawnProcess more complicated as described #274 (review), so ok to drop StreamSocketId.
There are some reasons why StreamSocketId might be useful: it is an inverse to the MakeStream function, and could be used in the EventLoop constructor to assign m_post_writer. But there isn't a need for it now.
112 | + // when one side of a socket pair is closed the other side may not receive 113 | + // these messages, preventing the remote side from freeing ProxyServer 114 | + // resources and shutting down cleanly. 115 | + try { 116 | + m_stream->shutdownWrite(); 117 | + } catch (const kj::Exception& e) {
In bfc2db7b517e59e2413b8920341d320bb1228dd2 proxy: Call shutdownWrite() in Connection destructor: maybe squash 28e4c7fd2eb992d632a3b394eab30d3e236208ca into this?
re: #274 (review)
In bfc2db7 proxy: Call shutdownWrite() in Connection destructor: maybe squash 28e4c7f into this?
Didn't squash these because I think 28e4c7fd2eb992d632a3b394eab30d3e236208ca is good documentation of the macos REDUCE_EXPORTS bug and will be probably be useful to reference in the future (the bug can happen very easily just by throwing and catching exceptions.)
Reviewed the rest as well, except the CMake changes in c9aa8060ec2a7b91d88bf5516320bfa41b14d4a5 and 7cb83a5d53476585c12dccabf84cc56294f64c5e.
Concept ACK
Thanks for the reviews!
<!-- begin push-2 -->
Rebased 7cb83a5d53476585c12dccabf84cc56294f64c5e -> 68ed129cc3c5803323ea75df28c806cd49f64710 (pr/wins.1 -> pr/wins.2, compare)<!-- end --> implementing review suggestions and fixing conflict with #279
<!-- begin push-3 -->
Updated 68ed129cc3c5803323ea75df28c806cd49f64710 -> a43e5a85bbf8f5551e41be8873c2c16b94ecd4c4 (pr/wins.2 -> pr/wins.3, compare)<!-- end --> to fix environ undeclared on macOS/BSD: add explicit declaration before posix_spawn() https://github.com/bitcoin-core/libmultiprocess/actions/runs/27974097183/job/82787313836
<!-- begin push-4 -->
Updated a43e5a85bbf8f5551e41be8873c2c16b94ecd4c4 -> 6cc729f9312641c0299b774228f89494dd85494a (pr/wins.3 -> pr/wins.4, compare)<!-- end --> to fix clang-tidy readability-redundant-declaration on environ declaration https://github.com/bitcoin-core/libmultiprocess/actions/runs/27978804911/job/82803323635
185 | - if (errno == ENOENT && !args.empty()) { 186 | - std::cerr << "Missing executable: " << fs::weakly_canonical(args.front()) << '\n'; 187 | - } 188 | - _exit(1); 189 | + ProcessId pid; 190 | + if (int err = posix_spawn(&pid, argv[0], nullptr, nullptr, argv.data(), ::environ)) {
In ddd3945be422d3b2addd2b25778eaff01ba41923 util, refactor: Do not fork() and exec() separately: not posix_spawnp?
re: #274 (review)
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
276 | @@ -276,6 +277,10 @@ std::tuple<ProcessId, SocketId> SpawnProcess(SpawnConnectInfoToArgsFn&& connect_ 277 | //! returning a socket id for communicating with the parent process. 278 | SocketId StartSpawned(const SpawnConnectInfo& connect_info); 279 | 280 | +//! Create a socket pair that can be used to communicate within a process or 281 | +//! between parent and child processes. 282 | +std::array<SocketId, 2> SocketPair();
In d9a5a68872167dd1bb0412d1174dcee694182527 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 (review)
In d9a5a68 util, refactor: Add SocketPair() and use it in SpawnProcess: it seems a bit odd to set
FD_CLOEXECand then unset it again for the child. The unset code also assume it was the only flag, since it sets0. 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
ACK 6cc729f9312641c0299b774228f89494dd85494a
I still didn't review the cmake commits though.
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 (review)
Good catch, added SocketId here
49 | @@ -51,14 +50,10 @@ int main(int argc, char** argv) 50 | std::cout << "Usage: mpcalculator <fd>\n"; 51 | return 1; 52 | } 53 | - mp::SocketId fd; 54 | - if (std::from_chars(argv[1], argv[1] + strlen(argv[1]), fd).ec != std::errc{}) { 55 | - std::cerr << argv[1] << " is not a number or is larger than an int\n"; 56 | - return 1; 57 | - } 58 | + 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 (review)
Didn't we lose our error handling here?
std::stoithrows 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
254 | @@ -255,6 +255,8 @@ std::string ThreadName(const char* exe_name); 255 | //! errors in python unit tests. 256 | std::string LogEscape(const kj::StringTree& string, size_t max_size); 257 | 258 | +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 (review)
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).
65 | @@ -67,10 +66,9 @@ void EventLoopRef::reset(bool relock) MP_NO_TSA 66 | loop->m_num_clients -= 1; 67 | if (loop->done()) { 68 | loop->m_cv.notify_all(); 69 | - int post_fd{loop->m_post_fd}; 70 | loop_lock->unlock(); 71 | char buffer = 0; 72 | - KJ_SYSCALL(write(post_fd, &buffer, 1)); // NOLINT(bugprone-suspicious-semicolon) 73 | + 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 (review)
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.
265 | @@ -266,10 +266,9 @@ void EventLoop::loop() 266 | m_task_set.reset(); 267 | MP_LOG(*this, Log::Info) << "EventLoop::loop bye."; 268 | wait_stream = nullptr; 269 | - KJ_SYSCALL(::close(post_fd)); 270 | const Lock lock(m_mutex); 271 | - m_wait_fd = SocketError; 272 | - m_post_fd = SocketError; 273 | + m_wait_stream = nullptr; 274 | + 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 (review)
Thanks, it does seem better to clear m_post_writer here for completeness, so added this
116 | + m_stream->shutdownWrite(); 117 | + } catch (const kj::Exception& e) { 118 | + // Ignore ENOTCONN: on macOS/FreeBSD (unlike Linux), shutdown(SHUT_WR) 119 | + // returns ENOTCONN if the peer already closed the connection. This is 120 | + // expected when the destructor is triggered by a remote disconnect. 121 | + 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 (review)
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
Code Review ACK c5faf68
Initial look at the chnages implemented here
Left a few suggestions and questions inline.
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.
<!-- begin push-5 -->
Rebased 6cc729f9312641c0299b774228f89494dd85494a -> 56220f87a55becd88904f4ba9eace9bb34a4bdf4 (pr/wins.4 -> pr/wins.5, compare)<!-- end --> due to conflicts, also applying review suggestions and dropping the cmake commits
<!-- begin push-6 -->
Updated 56220f87a55becd88904f4ba9eace9bb34a4bdf4 -> 491f1e80ea1b20ab549607e71ca0cf661ea0ffb7 (pr/wins.5 -> pr/wins.6, compare)<!-- end --> fixing comments and commit organization
<!-- begin push-7 -->
Rebased 491f1e80ea1b20ab549607e71ca0cf661ea0ffb7 -> 254c3e36372f0b53ed625d6e0b1d948df130da64 (pr/wins.6 -> pr/wins.7, compare)<!-- end --> due to conflict with #306
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>
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>
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.
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