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 through a cancel function. This is backed by a kj::Canceler that wraps the request promise and is attached to it. A canceled call throws 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. The first user of this mechanism is the cancellation parameter type std::function<void(std::function<void()>)>.

For example, this capnp schema method:

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

maps to a C++ method with a trailing cancellation argument, without requiring any libmultiprocess type in the interface header:

using CancelFn = std::function<void()>;
using CancelArg = std::function<void(CancelFn)>;

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

On the client, the argument receives a function that can cancel 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);
});

On the server, the implementation registers a callback that interrupts its wait:

std::unique_ptr<Template> waitNext(CancelArg cancel) override
{
    if (cancel) 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.

[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
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:

• #337 (proxy-types: Remove requirement for return types to be default-constructible by ryanofsky)
• #175 (Set cmake_minimum_required(VERSION 3.22) by maflcko)

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.

[ryanofsky]

why shared_ptr needs to be used

Looking over updated version of this (22000b4) 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.

[xyzconstant]

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.

[ryanofsky]

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.)

[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.

[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