Allow request cancellation for wrapped C++ methods #342

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](/bitcoin-core-multiprocess/contributor/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.

<!--e57a25ab6845829454e8d69fc972939a-->

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

<!--021abf342d371248e50ceaed478a90ca-->

Reviews

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

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

<!--174a7506f384e20aa4161008e828411d-->

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.

<!--5faf32d7da4f0f540f40219e4f7537a3-->

CI failures seem unrelated

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 https://github.com/bitcoin/bitcoin/issues/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 0986a13ea66a4fab1a8f64de5d37815efaaa1afa 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 3f6b324c6aae9722a947e514593a55bd85f4c7ef 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).

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.

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](/bitcoin-core-multiprocess/contributor/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.

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](/bitcoin-core-multiprocess/contributor/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](/bitcoin-core-multiprocess/contributor/7/) (context :Proxy.Context, timeoutMs :Int32) -> (result :Int32) $Proxy.extraParam("cancel");

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

Added 9422b97fc27dcf0946a9096f0cce2ab842e182e1, 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 f98bd9241cc2e6590fc5b20097f59d0ccad556f0. 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 :)

Thanks for the updates! And approach ACK f6944b2ba16de80a9a28f74ecbe9448f0896af03. 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.

why shared_ptr needs to be used

Looking over updated version of this (22000b4b072384170f7f88bd31a582970675c01b) I see that server side shared_ptr is gone, which is great. However, it looks like client-side shared_ptr is still necessary because when making the IPC call client side, we give the caller a CancelFn = std::function<void()> value which is copyable, because std::function values must be copyable. Therefore reference-counting is needed to determine the lifetime of the kj::Canceller object embedded in the CancelFn which is used to cancel the promise.

But I think the reference counting could be avoided by defining CancelFn instead as something like:

class CancelFn
{
    virtual ~CancelFn() = default;
    virtual void operator()() = 0;
};

and using CancelArg = std::function<void(const CancelFn&)> passing a reference to CancelFn instead of a copy. This way the implementation of CancelFn could just own the kj::Canceller object outright and not need reference counting.

I think doing this would also be good for type-safety. The current implementation of type-cancel will treat every argument of type std::function<void(std::function<void()>)> as a cancellation argument, even if that argument is intended to be used for something else. Using a stronger CancelFn type instead of a std::function alias should avoid interfering with arguments not related to cancellation.

Therefore reference-counting is needed to determine the lifetime of the kj::Canceller object embedded in the CancelFn which is used to cancel the promise.

But I think the reference counting could be avoided by defining CancelFn instead as something like:

class CancelFn
{
    virtual ~CancelFn() = default;
    virtual void operator()() = 0;
};

and using CancelArg = std::function<void(const CancelFn&)> passing a reference to CancelFn instead of a copy. This way the implementation of CancelFn could just own the kj::Canceller object outright and not need reference counting.

The CancelFn actually owns ClientCancelState, which can be safely reached from any arbitrary thread, unlike kj::Canceler. The promise owns the canceler object, and ClientCancelState contains only a pointer to it that answers "is there still a request to cancel?".

I think doing this would also be good for type-safety. The current implementation of type-cancel will treat every argument of type std::function<void(std::function<void()>)> as a cancellation argument, even if that argument is intended to be used for something else. Using a stronger CancelFn type instead of a std::function alias should avoid interfering with arguments not related to cancellation.

Note that an argument is treated as a cancellation argument only if the method also declares it with $Proxy.extraParam in the capnp schema. So, only declared extra parameters of that exact type are affected.

Also, an application can still declare its own overloads over stricter types and have access to invoke_context.cancel_receiver and server_context.cancel_fn. I'm not sure what to do here, honestly, I'm leaning toward keeping it as it is now.

I'm not sure what to do here, honestly, I'm leaning toward keeping it as it is now.

Yeah to be sure I'm just saying I think there may be an opportunity to simplify here, but the current approach seems fine. I do plan to review this, and if you are planning on working more on this I'd agree it's not very important to iterate on the exact types definitions here or avoid use of shared_ptr. I didn't see anything here that couldn't be merged as-is, and plan to review in more detail.

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. (I think you could even use the existing CancelArg type you defined in type-cancel.h for this in Bitcoin Core node code, since it can be forward declared and doesn't depend on any libmultiprocess headers, although it's not the most convenient type to use and ultimately a custom type might be more friendly for callers that want to trigger cancellations.)

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.

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

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

Thanks for the review @ryanofsky.

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

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.

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.

<details> <summary> Diff widening this race window with sleep and a test demonstrating the race </summary>

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;

</details>

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

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?

@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 2a2041238aa6f7abac650afe1287214fdc5178ce), 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.

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.

Thanks for the reviews and feedback, everyone!

Addressed feedback and force-pushed: 2a2041238aa6f7abac650afe1287214fdc5178ce -> 88e531b44c75272a35098879d7f0da5bd4d5e9f1 (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.

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

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

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.