Squashed 'src/ipc/libmultiprocess/' changes from 1868a84451f..789ac8c86d5

789ac8c86d5 test: m_on_cancel called after request finishes
5aa8c36cdd3 test: getParams() called after request cancel
7f954aa5eac race fix: m_on_cancel called after request finishes (bitcoin#34782)
88b4d85099d race fix: getParams() called after request cancel (bitcoin#34777)
4fa90c68015 race fix: worker thread destroyed before it is initialized (bitcoin#34711, bitcoin#34756)
22bec918c97 Merge bitcoin-core/libmultiprocess#247: type-map: Work around LLVM 22 "out of bounds index" error
8a5e3ae6ed2 Merge bitcoin-core/libmultiprocess#242: proxy-types: add CustomHasField hook to map Cap'n Proto values to null C++ values
e8d35246918 Merge bitcoin-core/libmultiprocess#246: doc: Bump version 8 > 9
97d877053b6 proxy-types: add CustomHasField hook for nullable decode paths
8c2f10252c9 refactor: add missing includes to mp/type-data.h
b1638aceb40 doc: Bump version 8 > 9
f61af487217 type-map: Work around LLVM 22 "out of bounds index" error

git-subtree-dir: src/ipc/libmultiprocess
git-subtree-split: 789ac8c86d5532438351e06296e7139565ba60d7

Author: ryanofsky

doc/versions.md

@@ -7,55 +7,68 @@ Library versions are tracked with simple
 Versioning policy is described in the [version.h](../include/mp/version.h)
 include.
 
-## v8
-- Current unstable version in master branch.
+## v9
+- Current unstable version.
 
-## v7.0
-- Adds SpawnProcess race fix, cmake `target_capnp_sources` option, ci and documentation improvements.
-- Used in Bitcoin Core master branch, pulled in by [#34363](https://github.com/bitcoin/bitcoin/pull/34363).
+## [v8.0](https://github.com/bitcoin-core/libmultiprocess/commits/v8.0)
+- Better support for non-libmultiprocess IPC clients: avoiding errors on unclean disconnects, and allowing simultaneous requests to worker threads which previously triggered "thread busy" errors.
+- Used in Bitcoin Core, pulled in by [#34422](https://github.com/bitcoin/bitcoin/pull/34422).
 
-## v7.0-pre1
+## [v7.0](https://github.com/bitcoin-core/libmultiprocess/commits/v7.0)
+- Adds SpawnProcess race fix, cmake `target_capnp_sources` option, ci and documentation improvements. Adds `version.h` header declaring major and minor version numbers.
+- Used in Bitcoin Core, pulled in by [#34363](https://github.com/bitcoin/bitcoin/pull/34363).
 
-- Adds support for log levels to reduce logging and "thread busy" error to avoid a crash on misuse. Fixes intermittent mptest hang and makes other minor improvements.
-- Used in Bitcoin Core 30.1 and 30.2 releases and 30.x branch. Pulled in by [#33412](https://github.com/bitcoin/bitcoin/pull/33412), [#33518](https://github.com/bitcoin/bitcoin/pull/33518), and [#33519](https://github.com/bitcoin/bitcoin/pull/33519).
+## [v7.0-pre2](https://github.com/bitcoin-core/libmultiprocess/commits/v7.0-pre2)
+- Fixes intermittent mptest hang and makes other minor improvements.
+- Used in Bitcoin Core 30.1 and 30.2 releases and 30.x branch, pulled in by [#33518](https://github.com/bitcoin/bitcoin/pull/33518) and [#33519](https://github.com/bitcoin/bitcoin/pull/33519).
 
-## v6.0
+## [v7.0-pre1](https://github.com/bitcoin-core/libmultiprocess/commits/v7.0-pre1)
 
-- Adds fixes for unclean shutdowns and thread sanitizer issues and adds CI scripts.
+- Adds support for log levels to reduce logging and "thread busy" error to avoid a crash on misuse.
+- Minimum required version for Bitcoin Core 30.1 and 30.2 releases and 30.x branch, pulled in by [#33412](https://github.com/bitcoin/bitcoin/pull/33412), [#33518](https://github.com/bitcoin/bitcoin/pull/33518), and [#33519](https://github.com/bitcoin/bitcoin/pull/33519).
+
+## [v6.0](https://github.com/bitcoin-core/libmultiprocess/commits/v6.0)
+
+- Adds CI scripts and build and test fixes.
+- Used in Bitcoin Core 30.0 release, pulled in by [#32345](https://github.com/bitcoin/bitcoin/pull/32345), [#33241](https://github.com/bitcoin/bitcoin/pull/33241), and [#33322](https://github.com/bitcoin/bitcoin/pull/33322).
+
+## [v6.0-pre1](https://github.com/bitcoin-core/libmultiprocess/commits/v6.0-pre1)
+
+- Adds fixes for unclean shutdowns and thread sanitizer issues.
 - Drops `EventLoop::addClient` and `EventLoop::removeClient` methods,
   requiring clients to use new `EventLoopRef` class instead.
-- Used in Bitcoin Core 30.0 release. Pulled in by [#31741](https://github.com/bitcoin/bitcoin/pull/31741), [#32641](https://github.com/bitcoin/bitcoin/pull/32641), [#32345](https://github.com/bitcoin/bitcoin/pull/32345), [#33241](https://github.com/bitcoin/bitcoin/pull/33241), and [#33322](https://github.com/bitcoin/bitcoin/pull/33322).
+- Minimum required version for Bitcoin Core 30.0 release, pulled in by [#31741](https://github.com/bitcoin/bitcoin/pull/31741), [#32641](https://github.com/bitcoin/bitcoin/pull/32641), and [#32345](https://github.com/bitcoin/bitcoin/pull/32345).
 
-## v5.0
+## [v5.0](https://github.com/bitcoin-core/libmultiprocess/commits/v5.0)
 - Adds build improvements and tidy/warning fixes.
 - Used in Bitcoin Core 29 releases, pulled in by [#31945](https://github.com/bitcoin/bitcoin/pull/31945).
 
-## v5.0-pre1
+## [v5.0-pre1](https://github.com/bitcoin-core/libmultiprocess/commits/v5.0-pre1)
 - Adds many improvements to Bitcoin Core mining interface: splitting up type headers, fixing shutdown bugs, adding subtree build support.
 - Broke up `proxy-types.h` into `type-*.h` files requiring clients to explicitly
   include overloads needed for C++ ↔️ Cap'n Proto type conversions.
 - Now requires C++ 20 support.
 - Minimum required version for Bitcoin Core 29 releases, pulled in by [#30509](https://github.com/bitcoin/bitcoin/pull/30509), [#30510](https://github.com/bitcoin/bitcoin/pull/30510), [#31105](https://github.com/bitcoin/bitcoin/pull/31105), [#31740](https://github.com/bitcoin/bitcoin/pull/31740).
 
-## v4.0
+## [v4.0](https://github.com/bitcoin-core/libmultiprocess/commits/v4.0)
 - Added better cmake support, installing cmake package files so clients do not
   need to use pkgconfig.
 - Used in Bitcoin Core 28 releases, pulled in by [#30490](https://github.com/bitcoin/bitcoin/pull/30490) and [#30513](https://github.com/bitcoin/bitcoin/pull/30513).
 
-## v3.0
+## [v3.0](https://github.com/bitcoin-core/libmultiprocess/commits/v3.0)
 - Dropped compatibility with Cap'n Proto versions before 0.7.
 - Used in Bitcoin Core 27 releases, pulled in by [#28735](https://github.com/bitcoin/bitcoin/pull/28735), [#28907](https://github.com/bitcoin/bitcoin/pull/28907), and [#29276](https://github.com/bitcoin/bitcoin/pull/29276).
 
-## v2.0
+## [v2.0](https://github.com/bitcoin-core/libmultiprocess/commits/v2.0)
 - Changed `PassField` function signature.
 - Now requires C++17 support.
 - Used in Bitcoin Core 25 and 26 releases, pulled in by [#26672](https://github.com/bitcoin/bitcoin/pull/26672).
 
-## v1.0
+## [v1.0](https://github.com/bitcoin-core/libmultiprocess/commits/v1.0)
 - Dropped hardcoded includes in generated files, now requiring `include` and
   `includeTypes` annotations.
 - Used in Bitcoin Core 22, 23, and 24 releases, pulled in by [#19160](https://github.com/bitcoin/bitcoin/pull/19160).
 
-## v0.0
+## [v0.0](https://github.com/bitcoin-core/libmultiprocess/commits/v0.0)
 - Initial version used in a downstream release.
 - Used in Bitcoin Core 21 releases, pulled in by [#16367](https://github.com/bitcoin/bitcoin/pull/16367), [#18588](https://github.com/bitcoin/bitcoin/pull/18588), and [#18677](https://github.com/bitcoin/bitcoin/pull/18677).

include/mp/proxy-types.h

@@ -23,9 +23,10 @@ class ValueField
     ValueField(Value&& value) : m_value(value) {}
     Value& m_value;
 
+    const Value& get() const { return m_value; }
     Value& get() { return m_value; }
     Value& init() { return m_value; }
-    bool has() { return true; }
+    bool has() const { return true; }
 };
 
 template <typename Accessor, typename Struct>
@@ -166,10 +167,52 @@ struct ReadDestUpdate
     Value& m_value;
 };
 
-template <typename... LocalTypes, typename... Args>
-decltype(auto) ReadField(TypeList<LocalTypes...>, Args&&... args)
+//! Return whether to read a C++ value from a Cap'n Proto field. Returning
+//! false can be useful to interpret certain Cap'n Proto field values as null
+//! C++ values when initializing nullable C++ std::optional / std::unique_ptr /
+//! std::shared_ptr types.
+//!
+//! For example, when reading from a `List(Data)` field into a
+//! `std::vector<std::shared_ptr<const CTransaction>>` value, it's useful to be
+//! able to interpret empty `Data` values as null pointers. This is useful
+//! because the Cap'n Proto C++ API does not currently provide a way to
+//! distinguish between null and empty Data values in a List[*], so we need to
+//! choose some Data value to represent null if we want to allow passing null
+//! pointers. Since no CTransaction is ever serialized as empty Data, it's safe
+//! to use empty Data values to represent null pointers.
+//!
+//! [*] The Cap'n Proto wire format actually does distinguish between null and
+//! empty Data values inside Lists, and the C++ API does allow distinguishing
+//! between null and empty Data values in other contexts, just not the List
+//! context, so this limitation could be removed in the future.
+//!
+//! Design note: CustomHasField() and CustomHasValue() are inverses of each
+//! other.  CustomHasField() allows leaving Cap'n Proto fields unset when C++
+//! types have certain values, and CustomHasValue() allows leaving C++ values
+//! unset when Cap'n Proto fields have certain values. But internally the
+//! functions get called in different ways. This is because in C++, unlike in
+//! Cap'n Proto not every C++ type is default constructible, and it may be
+//! impossible to leave certain C++ values unset. For example if a C++ method
+//! requires function parameters, there's no way to call the function without
+//! constructing values for each of the parameters. Similarly there's no way to
+//! add values to C++ vectors or maps without initializing those values.  This
+//! is not the case in Cap'n Proto where all values are optional and it's
+//! possible to skip initializing parameters and list elements.
+//!
+//! Because of this difference, CustomHasValue() works universally and can be
+//! used to disable BuildField() calls in every context, while CustomHasField()
+//! can only be used to disable ReadField() calls in certain contexts like
+//! std::optional and pointer contexts.
+template <typename... LocalTypes, typename Input>
+bool CustomHasField(TypeList<LocalTypes...>, InvokeContext& invoke_context, const Input& input)
+{
+    return input.has();
+}
+
+template <typename... LocalTypes, typename Input, typename... Args>
+decltype(auto) ReadField(TypeList<LocalTypes...>, InvokeContext& invoke_context, Input&& input, Args&&... args)
 {
-    return CustomReadField(TypeList<RemoveCvRef<LocalTypes>...>(), Priority<2>(), std::forward<Args>(args)...);
+    return CustomReadField(TypeList<RemoveCvRef<LocalTypes>...>(), Priority<2>(), invoke_context, std::forward<Input>(input), std::forward<Args>(args)...);
 }
 
 template <typename LocalType, typename Input>
@@ -190,6 +233,13 @@ void ThrowField(TypeList<std::exception>, InvokeContext& invoke_context, Input&&
     throw std::runtime_error(std::string(CharCast(data.begin()), data.size()));
 }
 
+//! Return whether to write a C++ value into a Cap'n Proto field. Returning
+//! false can be useful to map certain C++ values to unset Cap'n Proto fields.
+//!
+//! For example the bitcoin `Coin` class asserts false when a spent coin is
+//! serialized. But some C++ methods return these coins, so there needs to be a
+//! way to represent them in Cap'n Proto and a null Data field is a convenient
+//! representation.
 template <typename... Values>
 bool CustomHasValue(InvokeContext& invoke_context, const Values&... value)
 {
@@ -372,7 +422,7 @@ struct ClientException
         void handleField(InvokeContext& invoke_context, Results& results, ParamList)
         {
             StructField<Accessor, Results> input(results);
-            if (input.has()) {
+            if (CustomHasField(TypeList<Exception>(), invoke_context, input)) {
                 ThrowField(TypeList<Exception>(), invoke_context, input);
             }
         }

include/mp/proxy.h

@@ -70,6 +70,11 @@ struct ProxyContext
     Connection* connection;
     EventLoopRef loop;
     CleanupList cleanup_fns;
+    //! Hook called on the worker thread just before loop->sync() in PassField
+    //! for Context arguments. Used by tests to inject precise disconnect timing.
+    std::function<void()> testing_hook_before_sync;
+    //! Hook called on the worker thread just before returning results.
+    std::function<void()> testing_hook_after_cleanup;
 
     ProxyContext(Connection* connection);
 };

include/mp/type-context.h

@@ -61,8 +61,6 @@ auto PassField(Priority<1>, TypeList<>, ServerContext& server_context, const Fn&
         std::is_same<decltype(Accessor::get(server_context.call_context.getParams())), Context::Reader>::value,
         kj::Promise<typename ServerContext::CallContext>>::type
 {
-    const auto& params = server_context.call_context.getParams();
-    Context::Reader context_arg = Accessor::get(params);
     auto& server = server_context.proxy_server;
     int req = server_context.req;
     // Keep a reference to the ProxyServer instance by assigning it to the self
@@ -74,8 +72,10 @@ auto PassField(Priority<1>, TypeList<>, ServerContext& server_context, const Fn&
     auto self = server.thisCap();
     auto invoke = [self = kj::mv(self), call_context = kj::mv(server_context.call_context), &server, req, fn, args...](CancelMonitor& cancel_monitor) mutable {
                 MP_LOG(*server.m_context.loop, Log::Debug) << "IPC server executing request #" << req;
-                const auto& params = call_context.getParams();
-                Context::Reader context_arg = Accessor::get(params);
+                if (server.m_context.testing_hook_before_sync) server.m_context.testing_hook_before_sync();
+                // Save testing_hook_after_cleanup to a local because the
+                // server may be freed in the cleanup sync() below.
+                auto testing_hook_after_cleanup = std::move(server.m_context.testing_hook_after_cleanup);
                 ServerContext server_context{server, call_context, req};
                 {
                     // Before invoking the function, store a reference to the
@@ -127,6 +127,8 @@ auto PassField(Priority<1>, TypeList<>, ServerContext& server_context, const Fn&
                             server_context.request_canceled = true;
                         };
                         // Update requests_threads map if not canceled.
+                        const auto& params = call_context.getParams();
+                        Context::Reader context_arg = Accessor::get(params);
                         std::tie(request_thread, inserted) = SetThread(
                             GuardedRef{thread_context.waiter->m_mutex, request_threads}, server.m_context.connection,
                             [&] { return context_arg.getCallbackThread(); });
@@ -153,6 +155,15 @@ auto PassField(Priority<1>, TypeList<>, ServerContext& server_context, const Fn&
                         // the disconnect handler trying to destroy the thread
                         // client object.
                         server.m_context.loop->sync([&] {
+                            // Clear cancellation callback. At this point the
+                            // method invocation finished and the result is
+                            // either being returned, or discarded if a
+                            // cancellation happened. So we do not need to be
+                            // notified of cancellations after this point. Also
+                            // we do not want to be notified because
+                            // cancel_mutex and server_context could be out of
+                            // scope when it happens.
+                            cancel_monitor.m_on_cancel = nullptr;
                             auto self_dispose{kj::mv(self)};
                             if (erase_thread) {
                             // Look up the thread again without using existing
@@ -183,12 +194,15 @@ auto PassField(Priority<1>, TypeList<>, ServerContext& server_context, const Fn&
                     }
                     // End of scope: if KJ_DEFER was reached, it runs here
                 }
+                if (testing_hook_after_cleanup) testing_hook_after_cleanup();
                 return call_context;
             };
 
     // Lookup Thread object specified by the client. The specified thread should
     // be a local Thread::Server object, but it needs to be looked up
     // asynchronously with getLocalServer().
+    const auto& params = server_context.call_context.getParams();
+    Context::Reader context_arg = Accessor::get(params);
     auto thread_client = context_arg.getThread();
     auto result = server.m_context.connection->m_threads.getLocalServer(thread_client)
         .then([&server, invoke = kj::mv(invoke), req](const kj::Maybe<Thread::Server&>& perhaps) mutable {

include/mp/type-data.h

@@ -7,6 +7,9 @@
 
 #include <mp/util.h>
 
+#include <concepts>
+#include <span>
+
 namespace mp {
 template <typename T, typename U>
 concept IsSpanOf =

include/mp/type-function.h

@@ -48,13 +48,13 @@ struct ProxyCallFn
 };
 
 template <typename FnR, typename... FnParams, typename Input, typename ReadDest>
-decltype(auto) CustomReadField(TypeList<std::function<FnR(FnParams...)>>,
+decltype(auto) CustomReadField(TypeList<std::function<FnR(FnParams...)>> types,
     Priority<1>,
     InvokeContext& invoke_context,
     Input&& input,
     ReadDest&& read_dest)
 {
-    if (input.has()) {
+    if (CustomHasField(types, invoke_context, input)) {
         using Interface = typename Decay<decltype(input.get())>::Calls;
         auto client = std::make_shared<ProxyClient<Interface>>(
             input.get(), &invoke_context.connection, /* destroy_connection= */ false);

include/mp/type-interface.h

@@ -85,7 +85,7 @@ decltype(auto) CustomReadField(TypeList<std::unique_ptr<LocalType>>,
     typename Decay<decltype(input.get())>::Calls* enable = nullptr)
 {
     using Interface = typename Decay<decltype(input.get())>::Calls;
-    if (input.has()) {
+    if (CustomHasField(TypeList<LocalType>(), invoke_context, input)) {
         return read_dest.construct(
                                    CustomMakeProxyClient<Interface, LocalType>(invoke_context, std::move(input.get())));
     }
@@ -101,7 +101,7 @@ decltype(auto) CustomReadField(TypeList<std::shared_ptr<LocalType>>,
     typename Decay<decltype(input.get())>::Calls* enable = nullptr)
 {
     using Interface = typename Decay<decltype(input.get())>::Calls;
-    if (input.has()) {
+    if (CustomHasField(TypeList<LocalType>(), invoke_context, input)) {
         return read_dest.construct(
             CustomMakeProxyClient<Interface, LocalType>(invoke_context, std::move(input.get())));
     }

include/mp/type-map.h

@@ -27,6 +27,35 @@ void CustomBuildField(TypeList<std::map<KeyLocalType, ValueLocalType>>,
     }
 }
 
+// Replacement for `m.emplace(piecewise_construct, t1, t2)` to work around a
+// Clang 22 regression triggered by libc++'s std::map piecewise emplace: when
+// the key constructor argument tuple is empty (std::tuple<>), libc++'s internal
+// "try key extraction" SFINAE probe instantiates std::tuple_element<0,
+// std::tuple<>>, which Clang 22 diagnoses as an out-of-bounds pack access ("a
+// parameter pack may not be accessed at an out of bounds index") instead of
+// treating it as substitution failure. See LLVM issue #167709 and the upstream
+// fix in llvm/llvm-project PR #183614.
+// https://github.com/llvm/llvm-project/issues/167709
+// https://github.com/llvm/llvm-project/pull/183614
+template <class Map, class Tuple1, class Tuple2>
+auto EmplacePiecewiseSafe(
+    Map& m,
+    const std::piecewise_construct_t&,
+    Tuple1&& t1,
+    Tuple2&& t2)
+{
+    if constexpr (std::tuple_size_v<std::remove_reference_t<Tuple1>> == 0) {
+        // Avoid tuple<> / tuple<> (LLVM 22 libc++ regression path)
+        return m.emplace(std::piecewise_construct,
+                         std::forward_as_tuple(typename Map::key_type{}),
+                         std::forward<Tuple2>(t2));
+    } else {
+        return m.emplace(std::piecewise_construct,
+                         std::forward<Tuple1>(t1),
+                         std::forward<Tuple2>(t2));
+    }
+}
+
 template <typename KeyLocalType, typename ValueLocalType, typename Input, typename ReadDest>
 decltype(auto) CustomReadField(TypeList<std::map<KeyLocalType, ValueLocalType>>,
     Priority<1>,
@@ -42,7 +71,7 @@ decltype(auto) CustomReadField(TypeList<std::map<KeyLocalType, ValueLocalType>>,
                 Make<ValueField>(item),
                 ReadDestEmplace(
                     TypeList<std::pair<const KeyLocalType, ValueLocalType>>(), [&](auto&&... args) -> auto& {
-                        return *value.emplace(std::forward<decltype(args)>(args)...).first;
+                        return *EmplacePiecewiseSafe(value, std::forward<decltype(args)>(args)...).first;
                     }));
         }
     });