Skip to content

Commit 59123f5

Browse files
committed
feat: add push-callback registration for rcl primitives
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.
1 parent 9abcd5d commit 59123f5

6 files changed

Lines changed: 360 additions & 0 deletions

File tree

rclrs/src/client.rs

Lines changed: 25 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -468,6 +468,31 @@ where
468468
fn kind(&self) -> RclPrimitiveKind {
469469
RclPrimitiveKind::Client
470470
}
471+
472+
fn register_on_ready(
473+
&self,
474+
on_ready: Box<dyn Fn(ReadyKind, usize) + Send + Sync>,
475+
) -> Result<Option<Box<dyn crate::OnReadyHandle>>, RclrsError> {
476+
// A client has a single readiness path; report it as `Basic`.
477+
let on_ready = move |n| on_ready(ReadyKind::Basic, n);
478+
let registration = crate::executor::event_callback::OnReadyRegistration::new(
479+
Arc::clone(&self.handle),
480+
set_client_on_new_response,
481+
Box::new(on_ready),
482+
)?;
483+
Ok(Some(Box::new(registration)))
484+
}
485+
}
486+
487+
/// Install (or, with a null callback/user_data, clear) the "on new response"
488+
/// push callback used by the event-driven executor. Encapsulates the client
489+
/// lock and the rcl call within this module.
490+
unsafe fn set_client_on_new_response(
491+
handle: &ClientHandle,
492+
callback: rcl_event_callback_t,
493+
user_data: *const std::os::raw::c_void,
494+
) -> rcl_ret_t {
495+
rcl_client_set_on_new_response_callback(&*handle.lock(), callback, user_data)
471496
}
472497

473498
type SequenceNumber = i64;

rclrs/src/executor.rs

Lines changed: 2 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -1,6 +1,8 @@
11
mod basic_executor;
22
pub use self::basic_executor::*;
33

4+
pub(crate) mod event_callback;
5+
46
use crate::{
57
Context, ContextHandle, GuardCondition, IntoNodeOptions, Node, RclrsError, Waitable,
68
WeakActivityListener,
Lines changed: 243 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,243 @@
1+
//! Safe RAII wrapper around rcl's "on new ___" push-callback APIs
2+
//! (`rcl_subscription_set_on_new_message_callback` and the service/client/event
3+
//! equivalents).
4+
//!
5+
//! These let an event-driven executor learn that an entity has become ready
6+
//! *without* polling `rcl_wait`: the middleware invokes a C callback (possibly
7+
//! from its own thread) when data arrives. We forward that to a Rust closure,
8+
//! which an executor uses to enqueue work.
9+
//!
10+
//! [`OnReadyRegistration`] is generic over the entity handle type `H`. Each
11+
//! entity module provides a [`SetOnReadyFn`] that locks its handle and calls the
12+
//! appropriate `rcl_*_set_on_new_*_callback`; the registration owns the boxed
13+
//! callback context and the handle, and deregisters on drop.
14+
//!
15+
//! # Safety model
16+
//!
17+
//! rcl stores the `user_data` pointer we hand it and passes it back to the C
18+
//! callback on every event. That pointer must stay valid for as long as the
19+
//! callback is registered. We therefore:
20+
//!
21+
//! - box the [`EventCallbackCtx`] so it has a stable heap address, and
22+
//! - in `Drop`, **unregister the callback first** (so the middleware can no
23+
//! longer invoke the trampoline) and only then free the context.
24+
//!
25+
//! Getting that ordering wrong is a use-after-free, since the middleware may be
26+
//! calling the trampoline from another thread at the moment of teardown.
27+
28+
use std::{os::raw::c_void, sync::Arc};
29+
30+
use crate::{rcl_bindings::*, OnReadyHandle, RclrsError, ToResult};
31+
32+
/// The context carried through rcl as `user_data`. Boxed so its address is
33+
/// stable for the lifetime of the registration.
34+
struct EventCallbackCtx {
35+
on_ready: Box<dyn Fn(usize) + Send + Sync>,
36+
}
37+
38+
/// The C trampoline that rcl/rmw invokes when an entity becomes ready. It may be
39+
/// called from a middleware thread, so it does nothing but forward to the Rust
40+
/// closure. It must not run user code or take locks that could deadlock the
41+
/// middleware.
42+
unsafe extern "C" fn on_ready_trampoline(user_data: *const c_void, number_of_events: usize) {
43+
// SAFETY: `user_data` is the pointer to the `EventCallbackCtx` we passed to
44+
// the rcl setter. It stays valid until the owning registration's `Drop`
45+
// clears the callback, which always happens before the box is freed.
46+
let ctx = unsafe { &*(user_data as *const EventCallbackCtx) };
47+
(ctx.on_ready)(number_of_events);
48+
}
49+
50+
/// A function that registers (or, with a null callback/user_data, clears) the
51+
/// "on ready" push callback on an entity handle of type `H`. Implemented per
52+
/// entity module so that `H`'s (private) lock and its specific
53+
/// `rcl_*_set_on_new_*_callback` stay encapsulated there.
54+
pub(crate) type SetOnReadyFn<H> = unsafe fn(&H, rcl_event_callback_t, *const c_void) -> rcl_ret_t;
55+
56+
/// RAII registration of a push "on ready" callback on an rcl entity.
57+
///
58+
/// While alive, `on_ready(number_of_events)` is invoked by the middleware
59+
/// whenever the entity becomes ready. Dropping it unregisters the callback
60+
/// before releasing the context, so the middleware can never call into freed
61+
/// memory.
62+
pub(crate) struct OnReadyRegistration<H: Send + Sync + 'static> {
63+
set_callback: SetOnReadyFn<H>,
64+
// Field order is important for teardown safety: `handle` is declared
65+
// (and therefore dropped) before `ctx`. Dropping the last `Arc<handle>`
66+
// finalizes the rcl entity (destroying the middleware reader), so by the
67+
// time `ctx` is freed no middleware thread can still invoke the trampoline
68+
// against it. This mirrors rclcpp, which frees its callback storage only
69+
// after `rcl_*_fini`. See `Drop` below.
70+
handle: Arc<H>,
71+
72+
// Never read directly, held only so its `Drop` frees the callback context.
73+
#[allow(dead_code)]
74+
ctx: CtxBox,
75+
}
76+
77+
impl<H: Send + Sync + 'static> OnReadyHandle for OnReadyRegistration<H> {}
78+
79+
impl<H: Send + Sync + 'static> OnReadyRegistration<H> {
80+
/// Register `on_ready` to be called by the middleware whenever the entity
81+
/// becomes ready. `set_callback` locks `handle` and installs the trampoline.
82+
pub(crate) fn new(
83+
handle: Arc<H>,
84+
set_callback: SetOnReadyFn<H>,
85+
on_ready: Box<dyn Fn(usize) + Send + Sync>,
86+
) -> Result<Self, RclrsError> {
87+
let ctx = Box::into_raw(Box::new(EventCallbackCtx { on_ready }));
88+
89+
// SAFETY: `ctx` points to a live, heap-stable context that outlives the
90+
// registration (only freed once, when the `CtxBox` field is dropped).
91+
let result =
92+
unsafe { set_callback(&handle, Some(on_ready_trampoline), ctx as *const c_void).ok() };
93+
94+
if let Err(err) = result {
95+
// Registration failed, so nothing else references `ctx`. Reclaim it.
96+
// SAFETY: `ctx` came from `Box::into_raw` above and was never
97+
// successfully registered.
98+
unsafe {
99+
drop(Box::from_raw(ctx));
100+
}
101+
return Err(err);
102+
}
103+
104+
Ok(Self {
105+
set_callback,
106+
handle,
107+
ctx: CtxBox(ctx),
108+
})
109+
}
110+
}
111+
112+
impl<H: Send + Sync + 'static> Drop for OnReadyRegistration<H> {
113+
fn drop(&mut self) {
114+
// Detach the callback so the middleware stops invoking the trampoline.
115+
// The context is NOT freed here, the `ctx: CtxBox` field is dropped
116+
// *after* `handle`, so the rcl entity is finalized before
117+
// the context is freed, avoiding a use-after-free if a callback is still
118+
// in flight at teardown.
119+
//
120+
// SAFETY: handle is valid and locked by the setter. A null callback +
121+
// null user_data clears the registration.
122+
unsafe {
123+
let _ = (self.set_callback)(&self.handle, None, std::ptr::null());
124+
}
125+
}
126+
}
127+
128+
/// Owns the heap-allocated [`EventCallbackCtx`] and frees it on drop. Kept as a
129+
/// separate field of [`OnReadyRegistration`] so its drop runs *after* the
130+
/// `handle` Arc.
131+
struct CtxBox(*mut EventCallbackCtx);
132+
133+
// SAFETY: the pointer is only dereferenced by the middleware via the trampoline
134+
// (forwarding to a `Send + Sync` closure). It carries no thread-unsafe state.
135+
unsafe impl Send for CtxBox {}
136+
unsafe impl Sync for CtxBox {}
137+
138+
impl Drop for CtxBox {
139+
fn drop(&mut self) {
140+
// SAFETY: by the time this runs, `OnReadyRegistration::drop` has cleared
141+
// the callback and the `handle` field has been dropped (finalizing the
142+
// rcl entity if it was the last reference), so no middleware thread can
143+
// still be dereferencing this context. Reclaim it exactly once.
144+
unsafe {
145+
drop(Box::from_raw(self.0));
146+
}
147+
}
148+
}
149+
150+
#[cfg(test)]
151+
mod tests {
152+
use super::*;
153+
use crate::{subscription::set_subscription_on_new_message, *};
154+
use ros_env::test_msgs::msg;
155+
use std::{
156+
sync::atomic::{AtomicUsize, Ordering},
157+
time::{Duration, Instant},
158+
};
159+
160+
/// The push callback fires when messages arrive, without ever spinning the
161+
/// executor (i.e. without `rcl_wait`).
162+
#[test]
163+
fn push_callback_fires_without_spinning() -> Result<(), RclrsError> {
164+
let executor = Context::default().create_basic_executor();
165+
let node = executor.create_node(&format!("test_push_callback_{}", line!()))?;
166+
let qos = QoSProfile::default().reliable().keep_last(10);
167+
168+
let publisher = node.create_publisher::<msg::Empty>("test_push_topic".qos(qos))?;
169+
let subscription = node
170+
.create_subscription::<msg::Empty, _>("test_push_topic".qos(qos), |_: msg::Empty| {})?;
171+
172+
let count = Arc::new(AtomicUsize::new(0));
173+
let count_cb = Arc::clone(&count);
174+
let _registration = OnReadyRegistration::new(
175+
Arc::clone(subscription.handle()),
176+
set_subscription_on_new_message,
177+
Box::new(move |n| {
178+
count_cb.fetch_add(n, Ordering::Relaxed);
179+
}),
180+
)?;
181+
182+
// Publish repeatedly (to ride out discovery) and wait for the push
183+
// callback to fire. We deliberately never spin the executor.
184+
let deadline = Instant::now() + Duration::from_secs(10);
185+
while count.load(Ordering::Relaxed) == 0 && Instant::now() < deadline {
186+
publisher.publish(msg::Empty::default())?;
187+
std::thread::sleep(Duration::from_millis(20));
188+
}
189+
190+
assert!(
191+
count.load(Ordering::Relaxed) > 0,
192+
"push callback never fired"
193+
);
194+
Ok(())
195+
}
196+
197+
/// Rapidly create and drop registrations while messages are flowing. If the
198+
/// drop ordering is wrong (freeing the context before unregistering), the
199+
/// middleware thread can call into freed memory; this stresses that path.
200+
#[test]
201+
fn rapid_register_unregister_is_sound() -> Result<(), RclrsError> {
202+
let executor = Context::default().create_basic_executor();
203+
let node = executor.create_node(&format!("test_push_raii_{}", line!()))?;
204+
let qos = QoSProfile::default().reliable().keep_last(10);
205+
206+
let publisher = node.create_publisher::<msg::Empty>("test_push_raii_topic".qos(qos))?;
207+
let subscription = node.create_subscription::<msg::Empty, _>(
208+
"test_push_raii_topic".qos(qos),
209+
|_: msg::Empty| {},
210+
)?;
211+
212+
// A background thread floods the topic the whole time.
213+
let stop = Arc::new(std::sync::atomic::AtomicBool::new(false));
214+
let stop_pub = Arc::clone(&stop);
215+
let flood = std::thread::spawn(move || {
216+
while !stop_pub.load(Ordering::Acquire) {
217+
let _ = publisher.publish(msg::Empty::default());
218+
std::thread::sleep(Duration::from_micros(50));
219+
}
220+
});
221+
222+
// Register/unregister many times against the live subscription.
223+
for _ in 0..2000 {
224+
let count = Arc::new(AtomicUsize::new(0));
225+
let count_cb = Arc::clone(&count);
226+
let registration = OnReadyRegistration::new(
227+
Arc::clone(subscription.handle()),
228+
set_subscription_on_new_message,
229+
Box::new(move |n| {
230+
count_cb.fetch_add(n, Ordering::Relaxed);
231+
}),
232+
)?;
233+
// Hold briefly so the middleware can fire into this context, then drop
234+
// (which must unregister before freeing).
235+
std::thread::sleep(Duration::from_micros(100));
236+
drop(registration);
237+
}
238+
239+
stop.store(true, Ordering::Release);
240+
flood.join().unwrap();
241+
Ok(())
242+
}
243+
}

rclrs/src/service.rs

Lines changed: 25 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -307,6 +307,31 @@ where
307307
fn handle(&self) -> RclPrimitiveHandle<'_> {
308308
RclPrimitiveHandle::Service(self.handle.lock())
309309
}
310+
311+
fn register_on_ready(
312+
&self,
313+
on_ready: Box<dyn Fn(ReadyKind, usize) + Send + Sync>,
314+
) -> Result<Option<Box<dyn crate::OnReadyHandle>>, RclrsError> {
315+
// A service has a single readiness path; report it as `Basic`.
316+
let on_ready = move |n| on_ready(ReadyKind::Basic, n);
317+
let registration = crate::executor::event_callback::OnReadyRegistration::new(
318+
Arc::clone(&self.handle),
319+
set_service_on_new_request,
320+
Box::new(on_ready),
321+
)?;
322+
Ok(Some(Box::new(registration)))
323+
}
324+
}
325+
326+
/// Install (or, with a null callback/user_data, clear) the "on new request"
327+
/// push callback used by the event-driven executor. Encapsulates the service
328+
/// lock and the rcl call within this module.
329+
pub(crate) unsafe fn set_service_on_new_request(
330+
handle: &ServiceHandle,
331+
callback: rcl_event_callback_t,
332+
user_data: *const std::os::raw::c_void,
333+
) -> rcl_ret_t {
334+
rcl_service_set_on_new_request_callback(&*handle.lock(), callback, user_data)
310335
}
311336

312337
// SAFETY: The functions accessing this type, including drop(), shouldn't care about the thread

rclrs/src/subscription.rs

Lines changed: 35 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -103,6 +103,16 @@ where
103103
self.handle.topic_name()
104104
}
105105

106+
/// Access the handle for this subscription's underlying `rcl_subscription_t`.
107+
///
108+
/// Returns the subscription handle. Only the `event_callback` tests use this
109+
/// accessor (production code reaches the handle through its own field), so it
110+
/// is compiled only under test rather than carried as dead code.
111+
#[cfg(test)]
112+
pub(crate) fn handle(&self) -> &Arc<SubscriptionHandle> {
113+
&self.handle
114+
}
115+
106116
/// Returns the QoS settings of the subscription.
107117
pub fn qos(&self) -> QoSProfile {
108118
let options = unsafe {
@@ -294,6 +304,31 @@ where
294304
fn handle(&self) -> RclPrimitiveHandle<'_> {
295305
RclPrimitiveHandle::Subscription(self.handle.lock())
296306
}
307+
308+
fn register_on_ready(
309+
&self,
310+
on_ready: Box<dyn Fn(ReadyKind, usize) + Send + Sync>,
311+
) -> Result<Option<Box<dyn crate::OnReadyHandle>>, RclrsError> {
312+
// A subscription has a single readiness path; report it as `Basic`.
313+
let on_ready = move |n| on_ready(ReadyKind::Basic, n);
314+
let registration = crate::executor::event_callback::OnReadyRegistration::new(
315+
Arc::clone(&self.handle),
316+
set_subscription_on_new_message,
317+
Box::new(on_ready),
318+
)?;
319+
Ok(Some(Box::new(registration)))
320+
}
321+
}
322+
323+
/// Install (or, with a null callback/user_data, clear) the "on new message"
324+
/// push callback used by the event-driven executor. Encapsulates the
325+
/// subscription lock and the rcl call within this module.
326+
pub(crate) unsafe fn set_subscription_on_new_message(
327+
handle: &SubscriptionHandle,
328+
callback: rcl_event_callback_t,
329+
user_data: *const std::os::raw::c_void,
330+
) -> rcl_ret_t {
331+
rcl_subscription_set_on_new_message_callback(&*handle.lock(), callback, user_data)
297332
}
298333

299334
// SAFETY: The functions accessing this type, including drop(), shouldn't care about the thread

0 commit comments

Comments
 (0)