Skip to content

feat: add push-callback registration for rcl primitives (Tokio Executor 1/4)#653

Draft
azerupi wants to merge 2 commits into
ros2-rust:mainfrom
azerupi:pr/1-push-callbacks
Draft

feat: add push-callback registration for rcl primitives (Tokio Executor 1/4)#653
azerupi wants to merge 2 commits into
ros2-rust:mainfrom
azerupi:pr/1-push-callbacks

Conversation

@azerupi

@azerupi azerupi commented Jun 21, 2026

Copy link
Copy Markdown
Contributor

This is part of a set of PRs that attempt to add an event-driven Tokio executor to rclrs. The goal is to have something that:

  1. Works in a similar way than the events executor in rclcpp
  2. Interoperates nicely with the tokio ecosystem

I tried to split the work up into multiple PRs that build on top of each other to make the reviewing easier.

  1. feat: add push-callback registration for rcl primitives (Tokio Executor 1/4) #653
  2. feat: add an event-driven multi-threaded Tokio executor (Tokio Executor 2/4) #654
  3. feat: support actions on the Tokio executor (Tokio Executor 3/4) #655
  4. feat: add a shared timer scheduler for high-rate timers (Tokio Executor 4/4) #657

To make the CI work reliably for the Tokio event executor changes we need the following PRs to be merged:

  1. fix: action server clock use-after-free #651
  2. fix: make the test log handler coexist with rosout #656

Disclaimer
This work was heavily helped by the use of AI. That helped me clear a lot of ground quickly, but I want to be honest about it.

I'm submitting this set of PRs as a draft to get eyes on it and have other people help test, find limitations and flaws and provide feedback on how to improve it in order to reach the quality to be able to merge this.

add push-callback registration for rcl primitives

This PR adds the plumbing for event-driven readiness. Nothing in the existing executors consumes it yet, so behavior for users of the basic or polling Tokio executor is unchanged.

rcl can notify us when an entity becomes ready by calling a C callback, rather than us discovering it through rcl_wait. The relevant entry points are rcl_subscription_set_on_new_message_callback and the service and client equivalents. This PR wraps those in a safe Rust API.

RclPrimitive gains a register_on_ready method. You pass a closure to it and the implementation installs the rcl push callback and returns an OnReadyHandle. While that handle is alive, the middleware invokes your closure with a ReadyKind and a number_of_events count whenever the entity becomes ready.

Dropping the handle deregisters the callback. Primitives with no push API (timers, guard conditions) return Ok(None) so an executor can drive them another way.

The delicate part is teardown. rcl keeps the user_data pointer we register and passes it back on every notification, possibly from a middleware thread. That pointer must stay valid for as long as the callback is registered.
OnReadyRegistration boxes the callback context for a stable address and, on drop, clears the rcl callback before freeing the context.

register_on_ready is implemented for subscriptions, services, and clients. Actions are added in a follow-up PR.

Open questions

Changelog

2026-07-11 Catch panics from callbacks to avoid unwrapping across FFI

Wrap user closures in catch_unwind so a panic cannot unwind across the extern "C" boundary. Disable the registration after the first panic, log once, and expose an optional hook for executors to surface the error.

@azerupi azerupi changed the title feat: add push-callback registration for rcl primitives feat: add push-callback registration for rcl primitives (Tokio executor) Jun 21, 2026
@azerupi azerupi changed the title feat: add push-callback registration for rcl primitives (Tokio executor) feat: add push-callback registration for rcl primitives (Tokio Executor 1/3) Jun 21, 2026
Add an opt-in way for primitives to report readiness via rcl's push
callbacks (rcl_*_set_on_new_*_callback) instead of being polled in a wait
set, as the foundation for an event-driven executor.

`RclPrimitive::register_on_ready` installs a callback that the middleware
invokes when the entity becomes ready and returns an `OnReadyHandle`
(RAII) that deregisters on drop. `OnReadyRegistration` wraps the unsafe
rcl setter: it boxes the callback context for a stable address and, on
drop, clears the callback before freeing the context (finalizing the rcl
entity first) so the middleware can never invoke a freed context during
teardown.

Implemented for subscriptions, services, and clients. No executor consumes
this yet, so the basic executor is unchanged.
@azerupi azerupi force-pushed the pr/1-push-callbacks branch from 6809137 to 59123f5 Compare June 28, 2026 14:59
@azerupi azerupi changed the title feat: add push-callback registration for rcl primitives (Tokio Executor 1/3) feat: add push-callback registration for rcl primitives (Tokio Executor 1/4) Jun 28, 2026
@azerupi

azerupi commented Jul 11, 2026

Copy link
Copy Markdown
Contributor Author

Question: Can the callback be called if the rcl registration fails?

We are registering a callback in OnReadyRegistration::new with some context that we allocate. However if the registration returns an error we drop the context.

But it is not clear if when for example rcl_subscription_set_on_new_message_callback returns an error that it is guaranteed the callback will never be called. Because if it does get called and we dropped the context this ends in a use-after-free.

The rcl documentation seems to imply that error paths would not register the callback. (e.g. subscriptions, clients)

But in practice, there seem to be some deviation from this. For example in Jazzy it looks like CycloneDDS stores the callback and then could return RMW_RET_ERROR and then rcl forwards the error.

How are discrepancies between the rcl API documentation and actual implementations usually handled? Do we implement for the "spec" and document known violations or do we try to be more defensive in the rclrs implementations?

Wrap user closures in catch_unwind so a panic cannot unwind across the
extern "C" boundary. Disable the registration after the first panic,
log once, and expose an optional hook for executors to surface the error.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant