Allow request cancellation for wrapped C++ methods

[xyzconstant]

Currently, libmultiprocess requests are blocking, with no way to cancel them from either the client or server side. Downstream code works around this by pairing each blocking method with a dedicated interruptX() method whose only purpose is to wake it up. For example, bitcoin/bitcoin#33676 introduced BlockTemplate::interruptWait() specifically to wake an in-progress BlockTemplate::waitNext() call.

Cap'n Proto provides a useful cancellation mechanism that libmultiprocess can use to support cancellation from both non-libmultiprocess and libmultiprocess clients. When a promise is dropped, it sends a cancellation request. The server may either cancel immediately or ignore the signal.

This PR implements approach 4 from bitcoin/bitcoin#33575, adding cancellation support on both sides:

• Allow ProxyClient method calls to be canceled from another thread. ClientInvokeContext provides two hooks an application overload can set: set_canceler, which receives the kj::Canceler that clientInvoke wraps the request promise in, and handle_error, which receives the kj::Exception from a failed request and decides what the caller sees. The second hook is how a cancel parameter turns the DISCONNECTED that a kj::Canceler raises into InterruptException.
• Allow wrapped server methods to register a callback that runs when the request is canceled, whether because the promise was dropped or the connection was interrupted.

The PR also adds a new capnp annotation, $Proxy.extraParam, which declares C++-only parameters that are not sent through RPC. These parameters are handled by CustomBuildExtraParam on the client side and CustomReadExtraParam on the server side. Paired with the cancellation mechanism, this feature lets applications define their own cancel parameter types. A reference implementation lives under test/mp/test/foo-types.h and test/mp/test/foo.h, used in tests.

For example, given the following capnp schema method:

waitNext @0 (context :Proxy.Context) -> (result :Template) $Proxy.extraParam("cancel") $Cxx.allowCancellation;

and this C++ method with an application-defined cancel parameter:

using CancelFn = std::function<void()>;
class CancelGuard;   // unregisters the callback when destroyed
using CancelArg = std::function<CancelGuard(CancelFn)>;

virtual std::unique_ptr<Template> waitNext(CancelArg cancel) = 0;

The caller (client) passes a callback that receives the function that cancels the call:

CancelFn cancel_fn;

// ... blocks until the result arrives or another thread runs cancel_fn()
auto tmpl = client->waitNext([&](CancelFn fn) {
    cancel_fn = std::move(fn);
    return CancelGuard{};
});

The callee (server) registers a callback that interrupts its wait and retains the returned guard until the wait ends:

std::unique_ptr<Template> waitNext(CancelArg cancel) override
{
    CancelGuard guard;
    if (cancel) guard = cancel([this] { m_cv.notify_all(); });

    // ... wait on m_cv, checking for cancellation
}

Detecting a dropped promise on the server requires the $Cxx.allowCancellation annotation on the method, file, or interface. Without it, Cap'n Proto runs the abandoned call to completion. The annotation requires Cap'n Proto 1.0 (see the "Breaking change" section in https://capnproto.org/news/2023-07-28-capnproto-1.0.html), which this PR also sets as the minimum supported version.

---

NOTE: bitcoin/bitcoin#36097 builds on this PR by adding cancel arguments to the blocking mining methods (waitTipChanged, createNewBlock, BlockTemplate::waitNext) and deprecating the interrupt()/interruptWait() workarounds.

[DrahtBot]

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

Reviews

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

Type	Reviewers
Concept ACK	enirox001, ViniciusCestarii
Approach ACK	ryanofsky

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

Conflicts

Reviewers, this pull request conflicts with the following ones:

• #362 (Enable -Wunused by fanquake)
• #337 (proxy-types: Remove requirement for return types to be default-constructible by ryanofsky)

If you consider this pull request important, please also help to review the conflicting pull requests. Ideally, start with the one that should be merged first.

[xyzconstant]

CI failures seem unrelated

[ryanofsky]

Nice work! I've had a number of ideas about this feature over the years so it's really interesting to see it implemented. I will just give some quick thoughts for now so I don't get nerdsniped and wind up spending all day or more on this. Thoughts:

• I don't think the proxy.capnp Cancel struct should exist. The idea of allowing a CancelToken / interfaces::Cancel argument from RFC: Cancelling waitNext calls in the IPC mining interface bitcoin/bitcoin#33575 is just to provide a way for libmultiprocess C++ clients to cancel capnp::Response promises, and for libmultiprocess C++ servers to detect whether the promises they are fulfilling have been cancelled. Rust and python and non-libmultiprocess C++ clients and servers already have a way to do these things, so they should not be affected by implemntation of this feature, and not see any differences in .capnp schema files if a C++ method supports it or doesn't support it.

• Relatedly, the implementation should be orthogonal to the $Cxx.allowCancellation(true); annotation. The annotation controls how capnproto sends cancellations, while CancelToken is way of letting libmultiprocess C++ classes send and receive them. CancelToken will probably be most useful combined with $Cxx.allowCancellation(true); annotations, but it could be useful without them too, for example to interrupt calls on unclean disconnects, but not interrupt calls when clients drop capnp::Response promises because they don't need the results.

• In order for this feature to be useful in Bitcoin Core, it shouldn't require C++ interfaces to directly use the mp::CancelToken type. Bitcoin Core C++ interfaces in src/interfaces/ are intended to be used by node/wallet/gui code and compile without any dependency on libmultiprocess. So libmultiprocess could provide CustomBuildCancel and CustomReadCancel overloads analagous to CustomBuildField and CustomReadField overloads that applications can override to work with custom cancellation arguments. Probably the simplest cancellation argument type would look like:

  using CancelFn = std::function<void()>; // Called when a request is cancelled
  using CancelArg = std::function<void(CancelFn)>; // Called to set a CancelFn that is called when a request is cancelled.

  and support overloads like

  template <typename LocalType, typename Value>
  void CustomBuildCancel(TypeList<CancelArg>, InvokeContext& invoke_context, Value&& value)
  {
      // If client provied a CancelArg argument, call it to give them a CancelFn
      // callback they can use to interrupt this request.
      if (value) value([&invoke_context] { invoke_context.cancel_request(); } };
  }
  
  template <typename LocalType, typename ReadDest>
  decltype(auto) CustomReadCancel(TypeList<CancelArg>, InvokeContext& invoke_context, ReadDest&& read_dest)
  {
      // Return a CancelArg for servers call to register a CancelFn and be
      // notified if the current request is cancelled.
      return read_dest.construct([&invoke_context](CancelFn cancel_fn) invoke_context.on_request_cancelled(std::move(cancel_fn)); });
  }
  ``
  
  Having CustomBuildCancel/CustomReadCancel hooks would let libmultiprocess be agnostic to whatever cancellation interfaces C++ applications want to use, and just give clients a way to send cancellations and servers a way to receive them.
  

• Alternately instead of adding CustomBuildCancel/CustomReadCancel hooks, we could use existing CustomBuildField/CustomReadField hooks with empty input/output arguments. I think to do this would need to add a new hook called something like CustomFieldExists() that is constexpr and returns true by default but false if no capnproto field corresponding to the C++ type will exist. This would let clientInvoke/serverInvoke code handle C++ arguments that don't have corresponding capnproto fields, and be be similar to existing CustomHasField() / CustomHasValue() overloads but be constexpr and reflect whether the field exists at all, not whether it is has a value set.

• It looks like this implementation as of 0986a13 only allows libmultiprocess servers to detect cancellations, but doesn't allow libmultiprocess clients to request cancellations. This is ok, but it probably makes sense to support both because a single argument can support both, and so the feature can be more naturally tested end-to-end.

• Commit 3f6b324 is an interesting way to provide compatibility with older versions of capnproto that don't support $Cxx.allowCancellation annotations. I think in practice it's probably fine to drop support for these older versions though, and if we did want to support them we should probably just provide an $Cxx.allowCancellation annotation for them to use and call context.allowCancellation() when it's present.

• The CI failures in https://github.com/bitcoin-core/libmultiprocess/actions/runs/31636595747/job/94248350765?pr=342 are caused by this PR but they should be easy to fix. The -Wc++23-lambda-attributes errors are garbage from IWYU output but if you look below you will see real IWYU errors that need to be fixed. Once the IWYU errors are fixed all the IWYU output should disappear and CI should be green.

EDIT: Added CustomFieldExists idea above. Thinking about this more I believe it would be preferable to just add a new CustomFieldExists hook instead of adding CustomBuildCancel/CustomReadCancel hooks to be more general and also probably make the implementation simpler (just adding an argument handling option, not cancellation-specific argument handling options).

[xyzconstant]

Thanks for your feedback @ryanofsky. Sorry for the late response. I took some time to fill some C++ gaps (mostly to understand whether client-side detection of the CancelFn/CancelArg types could be solved through templating), since I'm still learning the language.

• I don't think the proxy.capnp Cancel struct should exist. The idea of allowing a CancelToken / interfaces::Cancel argument from RFC: Cancelling waitNext calls in the IPC mining interface bitcoin/bitcoin#33575 is just to provide a way for libmultiprocess C++ clients to cancel capnp::Response promises, and for libmultiprocess C++ servers to detect whether the promises they are fulfilling have been cancelled. Rust and python and non-libmultiprocess C++ clients and servers already have a way to do these things, so they should not be affected by implemntation of this feature, and not see any differences in .capnp schema files if a C++ method supports it or doesn't support it.

This makes complete sense. I believe I now grasp the original idea. This is cleaner for consumers and non-libmultiprocess clients, which don't need a Cancel schema parameter (it was only a marker for mpgen indicating the C++ method takes a token arg) because they already cancel their requests when promises are dropped.

• Relatedly, the implementation should be orthogonal to the $Cxx.allowCancellation(true); annotation. The annotation controls how capnproto sends cancellations, while CancelToken is way of letting libmultiprocess C++ classes send and receive them. CancelToken will probably be most useful combined with $Cxx.allowCancellation(true); annotations, but it could be useful without them too, for example to interrupt calls on unclean disconnects, but not interrupt calls when clients drop capnp::Response promises because they don't need the results.

Yes, you're correct here as well. $Cxx.allowCancellation only changes how a server responds to a cancel signal (whether the callee wants to cancel the request immediately or not). This means we can drop the dispatchCall override too.

• In order for this feature to be useful in Bitcoin Core, it shouldn't require C++ interfaces to directly use the mp::CancelToken type. Bitcoin Core C++ interfaces in src/interfaces/ are intended to be used by node/wallet/gui code and compile without any dependency on libmultiprocess. So libmultiprocess could provide CustomBuildCancel and CustomReadCancel overloads analagous to CustomBuildField and CustomReadField overloads that applications can override to work with custom cancellation arguments. Probably the simplest cancellation argument type would look like:

  using CancelFn = std::function<void()>; // Called when a request is cancelled
  using CancelArg = std::function<void(CancelFn)>; // Called to set a CancelFn that is called when a request is cancelled.

  and support overloads like

  template <typename LocalType, typename Value>
  void CustomBuildCancel(TypeList<CancelArg>, InvokeContext& invoke_context, Value&& value)
  {
      // If client provied a CancelArg argument, call it to give them a CancelFn
      // callback they can use to interrupt this request.
      if (value) value([&invoke_context] { invoke_context.cancel_request(); } };
  }
  
  template <typename LocalType, typename ReadDest>
  decltype(auto) CustomReadCancel(TypeList<CancelArg>, InvokeContext& invoke_context, ReadDest&& read_dest)
  {
      // Return a CancelArg for servers call to register a CancelFn and be
      // notified if the current request is cancelled.
      return read_dest.construct([&invoke_context](CancelFn cancel_fn) invoke_context.on_request_cancelled(std::move(cancel_fn)); });
  }

  Having CustomBuildCancel/CustomReadCancel hooks would let libmultiprocess be agnostic to whatever cancellation interfaces C++ applications want to use, and just give clients a way to send cancellations and servers a way to receive them.

This is interesting. I think we could combine this with OnCancel so it registers and unregisters the callback at the right time.

• Alternately instead of adding CustomBuildCancel/CustomReadCancel hooks, we could use existing CustomBuildField/CustomReadField hooks with empty input/output arguments. I think to do this would need to add a new hook called something like CustomFieldExists() that is constexpr and returns true by default but false if no capnproto field corresponding to the C++ type will exist. This would let clientInvoke/serverInvoke code handle C++ arguments that don't have corresponding capnproto fields, and be be similar to existing CustomHasField() / CustomHasValue() overloads but be constexpr and reflect whether the field exists at all, not whether it is has a value set.

I prefer this over the dedicated hooks. However, I don't fully understand how it would handle the client side. For clientInvoke to accept an argument without a field, the generated method has to take that argument in the first place (mpgen prints the parameter list based on the schema's field counts), and it must match the interface method exactly.

So I think the hook can tell clientInvoke what to do with the argument, but I don't see how it makes mpgen print a parameter the schema doesn't mention. Am I missing something here? If I'm not, I only see one way to handle this, and it's still making mpgen learn about the argument from the schema, this time not through a direct schema parameter like the Cancel struct but through a capnp method annotation.

• It looks like this implementation as of 0986a13 only allows libmultiprocess servers to detect cancellations, but doesn't allow libmultiprocess clients to request cancellations. This is ok, but it probably makes sense to support both because a single argument can support both, and so the feature can be more naturally tested end-to-end.

Yes, definitely we can cover both sides in this PR. I need to think about this more though, together with the mpgen issue above.

• Commit 3f6b324 is an interesting way to provide compatibility with older versions of capnproto that don't support $Cxx.allowCancellation annotations. I think in practice it's probably fine to drop support for these older versions though, and if we did want to support them we should probably just provide an $Cxx.allowCancellation annotation for them to use and call context.allowCancellation() when it's present.

Ok perfect. I'll drop it.

• The CI failures in https://github.com/bitcoin-core/libmultiprocess/actions/runs/31636595747/job/94248350765?pr=342 are caused by this PR but they should be easy to fix. The -Wc++23-lambda-attributes errors are garbage from IWYU output but if you look below you will see real IWYU errors that need to be fixed. Once the IWYU errors are fixed all the IWYU output should disappear and CI should be green.

Thanks, I really thought it wasn't caused by this PR, but I guess I had it wrong. I have some WIP rework locally, so these errors may be different the next time I push.

[ryanofsky]

This is interesting. I think we could combine this with OnCancel so it registers and unregisters the callback at the right time.

Hmm yeah. Actually when I wrote this I wasn't thinking about the need to unregister, and the CancelState and OnCancel interface seemed unnecessarily complicated to me. But unregistering is needed to deal with cancellation being sent of the the method finishes executing, so pretty important. (I still do think the vector of callbacks in CancelState might be too complicated though and a single std::function<void>() that could be set to null to unregister would probably be enough.)

I prefer this over the dedicated hooks. However, I don't fully understand how it would handle the client side. For clientInvoke to accept an argument without a field, the generated method has to take that argument in the first place (mpgen prints the parameter list based on the schema's field counts), and it must match the interface method exactly.

So I think the hook can tell clientInvoke what to do with the argument, but I don't see how it makes mpgen print a parameter the schema doesn't mention. Am I missing something here? If I'm not, I only see one way to handle this, and it's still making mpgen learn about the argument from the schema, this time not through a direct schema parameter like the Cancel struct but through a capnp method annotation.

You're right, I wasn't thinking about the client side of this very clearly. I was thinking as the client code was processing parameters it could use ProxyMethodTraits to see if the next parameter had a field mapping with CustomFieldExists. But this doesn't work at all because the code generator also needs to know the field mapping and doesn't have access to anything but the capnproto schema file.

So I think a capnproto method annotation of some kind might be necessary. I was thinking of adding annotations anyway to support more flexible mapping of c++ parameters to capnproto parameters like:

open @3 (name :Text $param(pos=1, type="CxxType")) -> (node :Node) $extraParam(name="foo",pos=2)) $extraParam(name="bar", pos=0]);

in context of #282. But something less general could also work well here.

[xyzconstant]

Hmm yeah. Actually when I wrote this I wasn't thinking about the need to unregister, and the CancelState and OnCancel interface seemed unnecessarily complicated to me. But unregistering is needed to deal with cancellation being sent of the the method finishes executing, so pretty important. (I still do think the vector of callbacks in CancelState might be too complicated though and a single std::function<void>() that could be set to null to unregister would probably be enough.)

The vector of callbacks exists because another callback (pre-existing) is registered in type-context.h to:

1. Log that the request was canceled mid-execution
2. Lock request_mutex so the event loop thread can't delete the params/results while the worker thread is still using them.

So during a cancellable request, there are two registrations: one internal, and another the wrapped method itself registers. But I do agree it looks a bit awkward. The raw pointers are there so OnCancel can own the callback by value and CancelState doesn't have to allocate, but there might be nicer ways to do that.

You're right, I wasn't thinking about the client side of this very clearly. I was thinking as the client code was processing parameters it could use ProxyMethodTraits to see if the next parameter had a field mapping with CustomFieldExists. But this doesn't work at all because the code generator also needs to know the field mapping and doesn't have access to anything but the capnproto schema file.

So I think a capnproto method annotation of some kind might be necessary. I was thinking of adding annotations anyway to support more flexible mapping of c++ parameters to capnproto parameters like:

open @3 (name :Text $param(pos=1, type="CxxType")) -> (node :Node) $extraParam(name="foo",pos=2)) $extraParam(name="bar", pos=0]);

in context of #282. But something less general could also work well here.

Good, this settles the client side then. I'd like to start with something minimal that we can scale up later into something more general.

I was thinking of a method annotation (your extraParam annotation but without the position parameter) that just names the extra C++ parameters that have no field:

waitValue @7 (context :Proxy.Context, timeoutMs :Int32) -> (result :Int32) $Proxy.extraParam("cancel");

[xyzconstant]

@ryanofsky Just addressed your comments, rebased with master, and pushed.

Added 9422b97, which supports the $Proxy.extraParam annotation, and built the cancellation feature on top of it. Also, bumped the minimum capnp version to 1.0 (mostly to pass in green olddeps) in f98bd92. If one of them seems like it needs to be moved to a separate PR, just let me know.

Updated the description too. Thanks for your feedback :)

[ryanofsky]

Thanks for the updates! And approach ACK f6944b2. I think it's good to add client and server cancellation support together in the same PR.

Quickly skimming changes, the extraParam implementation looks good and later changes also seem right, although I'm unclear in the later changes on why shared_ptr needs to be used, and why CancelState and ClientCancelState classes instead of plain std::function variables. I wonder if it might be possible to simplify more. But overall this looks good and I'm planning to review it.

It would also be great to see this put to use in bitcoin core by dropping the BlockTemplate::interruptWait and Mining::interrupt methods and supporting native request cancellation instead.

[xyzconstant]

In case you are looking for followup work, it'd be interesting to see a Bitcoin core PR taking advantage of this, for example replacing the interrupt/interruptWait methods as suggested above and allowing native cancellation of mining methods that wait.

Yes, sure. I’ve had Claude continuously testing the feature against Downstream source. I have some its changes locally, I just need to review and polish them before opening a PR.

Done at bitcoin/bitcoin#36097

[ryanofsky]

Code review 0d7a2ff. Starting to go through this, looks very good so far!

[ryanofsky]

In commit "Edited init.cpp to include a check that -datadir exists" (ed2c01405f5a5b913c4fd64397394559051e041a)

Can we change this to a static_assert like MaybeReadExtraParam below instead of discarding the argument passed by the client?

I think this would be safer because it's easy to define a CustomBuildExtraParam function with a slightly incorrect signature that could get silently skipped. And I think in practice for real use-cases, it should make sense to define CustomBuildExtraParam and CustomReadExtraParam functions in pairs, and easy to leave implementations empty if one of them is not needed.

[xyzconstant]

Done at 1eaa501

[ryanofsky]

In commit "Allow wrapped C++ methods to take parameters that are not sent over RPC" (88ea9fa)

These functions (unlike MaybeReadField and MaybeBuildField) are only called one place, and it would seem simpler and less confusing to just inline them where they are used.

[xyzconstant]

Thanks! done at 1eaa501

[ryanofsky]

In commit "Allow wrapped C++ methods to take parameters that are not sent over RPC" (88ea9fa)

Is there a specific reason this limitation was added? It seems like multiple extraparams could work without any extra support, so unclear why they are not allowed. Similar for the other checks, it doesn't really seem like it should be a problem for these parameters not to have names for the names not to be unique. The names seem like they can only exist for descriptive purposes, and there's not really anything meaningful we could do with them, so there doesn't seem to be a reason to require them or restrict them.

[xyzconstant]

The only reason was to ensure that only 1 extra parameter was allowed, and yes you're right. This works without extra support, so this restriction no longer makes sense. Thanks!

Done at 1eaa501

[ryanofsky]

In commit "Allow wrapped C++ methods to take parameters that are not sent over RPC" (88ea9fa)

Test could potentially verify that CustomBuildExtraParam is called and 999 value is received (testing_hook_misc from 914dc83 could work well for this)

[xyzconstant]

Nice, I added the misc hook at 1eaa501

[xyzconstant]

Thanks for the review @ryanofsky.

I addressed your feedback on the extraParam annotation support, squashed them and force-pushed (1eaa501).

[enirox001]

Concept ACK

IIUC, this allows one thread to cancel an IPC call that another thread is blocked waiting for, without closing the connection. The server method can also register a callback to respond to cancellation.

This seems complementary to #335 as thesse both provide mechanisms in libmultiprocess that reduce the need for custom interruption and shutdown handling in Bitcoin Core. That seems like a useful direction.

[ViniciusCestarii]

Concept ACK. Commented 3 nits.

I also used claude to review and it found that ServerCall::invoke cancellation is currently racy:

"The server unlocks request_lock for the whole method body and only reacquires it afterwards to clear cancel_fn. In the gap between the method returning (its frame, and anything the callback captured from it, destroyed) and that reacquire, the event loop can take the mutex, see cancel_fn still set, and invoke a callback whose captures are gone."

The race window seems pretty narrow but possible.

Diff widening this race window with sleep and a test demonstrating the race

diff --git a/include/mp/proxy-types.h b/include/mp/proxy-types.h
index 2172326..25edf50 100644
--- a/include/mp/proxy-types.h
+++ b/include/mp/proxy-types.h
@@ -7,8 +7,10 @@
 
 #include <mp/proxy-io.h>
 
+#include <chrono>
 #include <exception>
 #include <optional>
+#include <thread>
 #include <set>
 #include <typeindex>
 #include <vector>
@@ -608,6 +610,10 @@ struct ServerCall
                     extra);
             },
             [&] {
+                // TEMPORARY REPRO HACK: widen the existing window between the
+                // wrapped method returning (its locals already destroyed) and
+                // the worker thread reacquiring request_lock.
+                if (server_context.request_lock) std::this_thread::sleep_for(std::chrono::milliseconds{500});
                 if (server_context.request_lock) server_context.request_lock->m_lock.lock();
                 // The method returned, so destroy the callback it registered
                 // through its cancellation argument, if any.
diff --git a/test/mp/test/test.cpp b/test/mp/test/test.cpp
index 619930e..fefd141 100644
--- a/test/mp/test/test.cpp
+++ b/test/mp/test/test.cpp
@@ -816,6 +816,55 @@ KJ_TEST("Cancel an in-flight IPC call")
     KJ_EXPECT(foo->add(1, 2) == 3);
 }
 
+KJ_TEST("Cancel racing method return is not a use-after-free")
+{
+    TestSetup setup;
+    ProxyClient<messages::FooInterface>* foo = setup.client.get();
+    foo->initThreadMap();
+    std::promise<void> registered;
+    std::atomic<bool> callback_ran{false};
+    std::atomic<bool> ran_after_return{false};
+
+    // Register a cancel callback, then return immediately instead of waiting
+    // for cancellation. This targets the window in proxy-types.h between the
+    // wrapped method returning (its frame, and anything the callback captured
+    // from it, destroyed) and request_lock being reacquired to clear
+    // cancel_fn. The callback holds a weak_ptr to a frame-owned canary
+    // instead of a dangling reference, so it can observe that it was invoked
+    // after the frame died without actually corrupting memory.
+    setup.server->m_impl->m_cancel_fn = [&](CancelArg cancel) {
+        auto canary = std::make_shared<int>(42);
+        cancel([weak = std::weak_ptr<int>{canary}, &callback_ran, &ran_after_return] {
+            callback_ran = true;
+            if (weak.expired()) ran_after_return = true;
+        });
+        registered.set_value();
+    };
+
+    std::promise<CancelFn> cancel_fn;
+    std::thread canceler([&] {
+        CancelFn fire{cancel_fn.get_future().get()};
+        registered.get_future().wait();
+        // Let the method return before canceling, to land in the window.
+        std::this_thread::sleep_for(std::chrono::milliseconds{100});
+        fire();
+    });
+    bool interrupted = false;
+    try {
+        foo->callCancelFnAsync([&](CancelFn fn) { cancel_fn.set_value(std::move(fn)); });
+    } catch (const InterruptException&) {
+        interrupted = true;
+    }
+    canceler.join();
+    KJ_EXPECT(interrupted);
+    KJ_EXPECT(callback_ran);
+    // Fails today: the callback is invoked after the method's frame is gone.
+    KJ_EXPECT(!ran_after_return);
+
+    // Connection should be unaffected.
+    KJ_EXPECT(foo->add(1, 2) == 3);
+}
+
 KJ_TEST("Dropping the client promise cancels an executing method")
 {
     TestSetup setup;

A possible fix is having registration return an RAII handle that clears cancel_fn under the mutex.

[ViniciusCestarii]

In "Allow wrapped C++ methods to take parameters that are not sent over RPC" 1eaa501

nit: nested ternaries here are hard to read. Maybe:

    kj::StringPtr field_name;
    if (extra) {
        field_name = field.extra_name;
    } else {
        field_name = (field.param_is_set ? field.param : field.result).getProto().getName();
        add_accessor(field_name);
    }

which also folds in the if (!extra) add_accessor(...) on the next line.

[xyzconstant]

Nice suggestion, updated in c14d0d5.

[ViniciusCestarii]

In "Allow wrapped C++ methods to take parameters that are not sent over RPC" 1eaa501

nit: not sure if this is helpful. Arity mismatches are already compile errors without it, and since static_assert doesn't stop further diagnostics it only prepends a nicer first line rather than replacing the noise. It also fires only for methods with $Proxy.extraParam, while an ordinary method with the same mismatch gets nothing.

[xyzconstant]

Nice catch! I agree this is kind of redundant. Dropped this check in c14d0d5. Thanks!

[ViniciusCestarii]

In "Allow request cancellation for wrapped C++ methods" cfdcb45

nit: I found "cancel_receiver" a confusing name because it makes you think it receives cancellation but it is only a registration hook. A better name could be "register_cancel"

[xyzconstant]

Updated it to pass_cancel_fn in 298806f

[xyzconstant]

Thanks for the review @ViniciusCestarii, and nice catch on the ServerCall::invoke race.

"The server unlocks request_lock for the whole method body and only reacquires it afterwards to clear cancel_fn. In the gap between the method returning (its frame, and anything the callback captured from it, destroyed) and that reacquire, the event loop can take the mutex, see cancel_fn still set, and invoke a callback whose captures are gone."

So the fix must either clear cancel_fn within the wrapped method body before the locals are destroyed or expect the method implementor to pass ownership of the frame values to the cancel callback.

A possible fix is having registration return an RAII handle that clears cancel_fn under the mutex.

This implies that CancelArg must return a type other than void.

@ryanofsky, since the RAII handle requires changing CancelArg, this seems like a good time to revisit your earlier suggestion for a stronger CancelFn type rather than the std::function aliases. Thoughts?

[enirox001]

In 1eaa501: Allow wrapped C++ methods to take parameters that are not sent over RPC

nit:
Here, the extra argument passed by value here gets copied; I assume the problems with this have been considered since it was made to use an std::move in cfdcb45

But I think the move should be introduced in this commit instead

[xyzconstant]

Done at 780120c

[enirox001]

In 1eaa501: Allow wrapped C++ methods to take parameters that are not sent over RPC

We can improve how the generator identifies an extra parameter; currently it uses a non-empty name as a marker. Still, if there is an extra entry that is empty, $Proxy.extraParam("") that entry has no real capnp field, but the generator treats it as an ordinary field, because its name is empty; it then attempts to obtain a name from the nonexistent field.

The generator also exits successfully, but produces invalid c++

MakeClientParam<Accessor<empty_fields::, FIELD_OUT>>(M0::Fwd<1>())

I think we should be rejecting empty names with a clear error

[xyzconstant]

Thanks for spotting it. A check for this existed at 88ea9fa and threw an error if no name was provided. I removed it after Ryan's feedback (comment) since there's no real reason to require a name. But as you noted, mpgen was still relying on the name's length being > 0, and an empty extraParam fell through the normal capnp path.

Fixed in 780120c. It adds a new extra flag to the Field struct, sets it when it detects the annotation, and replaces the name-length check to initialize the local extra variable. The empty name turns into unnamed_extra_paramN.

[enirox001]

In commit 7f31996: proxy: rename cancel_lock and cancel_mutex to request_lock and request_mutex

This is a good change to use the naming request_lock, the previous name would have been confusing, as it could suggest that it controls whether cancellation happens. The new change better describes what it protects, which is the request data.

nit: there is a comment that I think you missed;

index 63b3fdb..c1ca04f 100644
--- a/include/mp/type-context.h
+++ b/include/mp/type-context.h
@@ -154,7 +154,7 @@ auto PassField(Priority<1>, TypeList<>, ServerContext& server_context, const Fn&
         // makes another IPC call), so avoid modifying the map.
         const bool erase_thread{inserted};
         KJ_DEFER(
-            // Release the cancel lock before calling loop->sync and
+            // Release the request lock before calling loop->sync and
             // waiting for the event loop thread, because if a
             // cancellation happened, it needs to run the on_cancel
             // callback above. It's safe to release request_lock at

[xyzconstant]

Thanks! Done in 2e5e66d

[enirox001]

In cfdcb45: proxy: support cancellation extra parameters

Could cancellation run the server callback after the local variables it references have been destroyed?

The callback is cleared only after the wrapped method returns, but the method’s local variables are destroyed before that. If cancellation arrives in between, the callback could access destroyed state—for example, a local mutex or condition variable.

I reproduced this with a test that pauses during method cleanup and triggers cancellation after the local state is destroyed. The callback still runs.

index 619930e..5ad02a0 100644
--- a/test/mp/test/test.cpp
+++ b/test/mp/test/test.cpp
@@ -816,6 +816,85 @@ KJ_TEST("Cancel an in-flight IPC call")
     KJ_EXPECT(foo->add(1, 2) == 3);
 }

+KJ_TEST("Cancellation callback does not outlive server method locals")
+{
+    TestSetup setup;
+    auto* foo = setup.client.get();
+    foo->initThreadMap();
+    constexpr std::chrono::seconds timeout{30};
+    std::promise<void> locals_destroyed;
+    auto locals_destroyed_future = locals_destroyed.get_future();
+    std::promise<void> cancellation_attempted;
+    auto cancellation_attempted_future = cancellation_attempted.get_future();
+    std::atomic<bool> callback_after_destruction{false};
+
+    setup.server->m_impl->m_cancel_fn = [&](CancelArg cancel) {
+        // This guard runs after local_state is destroyed, but before the
+        // method returns to ServerCall's callback cleanup. Hold that window
+        // open so cancellation reliably arrives during normal scope exit.
+        KJ_DEFER(
+            locals_destroyed.set_value();
+            KJ_EXPECT(cancellation_attempted_future.wait_for(timeout) == std::future_status::ready);
+        );
+        );
+            KJ_EXPECT(cancellation_attempted_future.wait_for(timeout) == std::future_status::ready);
+        );
+        auto local_state = std::make_shared<int>(42);
+        cancel([state = std::weak_ptr<int>{local_state}, &callback_after_destruction] {
+            // Observe the lifetime safely, instead of dereferencing a dangling
+            // reference to a local mutex/condition variable as real code might.
+            callback_after_destruction = state.expired();
+        });
+        // Return normally; do not wait for cancellation in the method body.
+    };
+
+    std::promise<CancelFn> cancel_fn;
+    auto cancel_future = cancel_fn.get_future();
+    std::thread canceler([&] {
+        CancelFn fire = cancel_future.get();
+        KJ_EXPECT(locals_destroyed_future.wait_for(timeout) == std::future_status::ready);
+        fire();
+        cancellation_attempted.set_value();
+    });
+    bool interrupted{false};
+    try {
+        foo->callCancelFnAsync([&](CancelFn fn) { cancel_fn.set_value(std::move(fn)); });
+    } catch (const InterruptException&) {
+        interrupted = true;
+    }
+    canceler.join();
+    KJ_EXPECT(interrupted);
+    KJ_EXPECT(!callback_after_destruction.load());
+    KJ_EXPECT(foo->add(1, 2) == 3);
+}
+

Should we provide a way to unregister the callback before the captured local variables go out of scope?

[xyzconstant]

@ViniciusCestarii reported the same issue and suggested an RAII guard that unregisters the cancel callback before the wrapped method's locals go out of scope.

Following @ryanofsky's suggestion in #342 (comment), it's a bit of a redesign. In the latest pushes, type-cancel.h was moved to test/, and now the source code changes are smaller than in your last review.

[enirox001]

In cfdcb45: proxy: support cancellation extra parameters

From the previous commit bf8a8b1, every ipc call now creates a ClientCancelState allocates a RequestCanceler and wraps the response promise even when the caller does not want cancellation. I think with this commit that connects cancel_receiver, we could have a way where this would happen only when a cancellation receiver is present. This would reduce the allocations for ordinary calls

[xyzconstant]

Thanks! Dropped this, and now clientInvoke checks whether the new field invoke_context.set_canceler is set before constructing a kj::Canceler object and attaching it to the request promise.

[enirox001]

In 527a099: build: require Cap'n Proto 1.0

nit: There is a text here that needs to be updated i think

index f2199cb..a6a8a0f 100644
--- a/CMakeLists.txt
+++ b/CMakeLists.txt
@@ -18,7 +18,7 @@ find_package(CapnProto 1.0 NO_MODULE)
    message(FATAL_ERROR
      "Cap'n Proto is required but was not found.\n"
      "To resolve, choose one of the following:\n"
-     "  - Install Cap'n Proto (version 1.0+ recommended)\n"
+     "  - Install Cap'n Proto (version 1.0+ required)\n"
      "  - For Bitcoin Core compilation build with -DENABLE_IPC=OFF to disable multiprocess support\n"
    )
  endif()

[xyzconstant]

This makes more sense, thanks. Done in 45514af.

[ryanofsky]

@ryanofsky, since the RAII handle requires changing CancelArg, this seems like a good time to revisit your earlier suggestion for a stronger CancelFn type rather than the std::function aliases. Thoughts?

Yes I'm a little behind on this PR (as of 2a20412), but my thinking is that this PR should not add a include/mp/type-cancel.h file, but should just add interfaces and tools needed to let clients like bitcoin core define their own custom cancellation argument types. The code in include/mp/type-cancel.h can be moved to test/ and used for unit tests though.

I am also a little suspicious of complexity I see in clientInvoke. I don't know know why for example kj::Canceller is wrapped in a RequestCanceler class instead of just being used directly, and why a direct pointer to the canceller isn't given the to the custom cancellation argument instead of kj::Canceller being wrapped in RequestCanceler being wrapped in a pass_cancel_fn callback. I also think std::make_shared in clientInvoke adds unnecessary overhead when most client IPC calls won't be cancellable. And the new throw InterruptException in clientInvoke add complexity and seems inflexible, because custom cancellation arguments should be able to throw their own custom exceptions when they trigger cancellation (or not throw exceptions at all if they want to indicate cancellation some other way).

I'd expect the clientInvoke implementation to be more minimal. Something more like:

• ClientInvokeContext struct could have a new std::function<void(kj::Canceler*)> set_canceler member set to null by default.
• If a cancellation argument is passed, CustomBuildExtraParam could set set_canceler to a callback to receive a pointer to the kj::Canceler object and hook it up to the cancellation argument.
• After building parameters, but before sending the request, clientInvoke could check if set_canceler is set, and if not, just do the normal IPC call with no extra overhead. If it is set, create a kj::Canceler instance and wrap the request in it, and call set_canceler to share it with the cancellation argument.
• After the request finishes, if set_canceler is not null, call set_canceler(nullptr) to so cancellation argument is informed the request is no longer cancellable, and previous kj::Canceler pointer is no longer valid.
• Everything else can be left to cancellation argument implementation. clientInvoke should not care whether the request was cancelled or not. A CustomReadExtraParam call in ClientParam<void>::ReadResults can let the cancellation argument decide to throw an exception if the request was cancelled.

I think this approach would reduce overhead and complexity and assign responsibilities better.

[ryanofsky]

A CustomReadExtraParam call in ClientParam<void>::ReadResults can let the cancellation argument decide to throw an exception if the request was cancelled.

Actually, this won't work because clientInvoke skips reading fields if the request is cancelled.

But this could be an opportunity to add support for custom exceptions on other types of events too (like disconnections) not just cancellations. Specifically could add some code to the client error handler to translate the const ::kj::Exception& e into a C++ exception:

                try {
                    IterateFields().handleChain(
                        *invoke_context, e, FieldList(), typename FieldObjs::HandleError{&fields}...);
                } catch (...) {
                    exception = std::current_exception();
                }

Alternately maybe this is too complicated and it simpler to throw InterruptException on all cancellations. This seems pretty limiting but on the other hand we don't currently have a need for client-side cancellation support on bitcoin core. Still in this simpler case, it should be possible to detect cancellation with e.getType() == kj::Exception::Type::CANCELED without needing cancel_state->canceled() complexity.

[xyzconstant]

Thanks for the reviews and feedback, everyone!

Addressed feedback and force-pushed: 2a20412 -> 88e531b (compare).

---

re: #342 (comment)

it should be possible to detect cancellation with e.getType() == kj::Exception::Type::CANCELED without needing cancel_state->canceled() complexity.

There is no CANCELED kind in kj::Exception::Type. In fact, kj::Canceler::cancel raises DISCONNECTED just like a normal disconnect. To handle that, a second hook, handle_error, was added to ClientInvokeContext to let applications handle kj::Exception failures. This way, an error handler paired with a cancel argument can check custom cancel state before throwing, like this:

    invoke_context.handle_error = [state](const kj::Exception&) {
        if (state->canceled()) throw InterruptException{"canceled"};
    };

re: #342 (comment)

I followed your suggestion, and I can confirm that the source code changes are smaller and more efficient, most notable changes are:

• type-cancel.h overloads (along with the old ClientCancelState, now called CancelFnState) were moved to test/mp/test/foo-types.
• The reference cancel parameter now returns CancelGuard.
• clientInvoke now checks if the method is cancelable via invoke_context.set_canceler and constructs a kj::Canceler object wrapping the request promise. This removes the cost that every non-cancelable method was paying unconditionally.

Thanks for the suggestion.

[xyzconstant]

Force-pushed ab05fdc (compare) to make the reference application-defined CancelGuard class [[nodiscard]] and move-assignable.

Also, updated the PR description to match the latest changes.

[xyzconstant]

FWIW: I just pushed changes to bitcoin/bitcoin#36097 to sync with this PR's current state. I also updated the description here to reference it.