rmw executor callbacks are invoked under a mutex; the ordinary startup path hits it

[YuanYuYuan]

Part of #282 — instance of the re-entrancy defect class; fixed by #262. The class, the shared fix shape and the merge order are stated in #282.

What a user does

Runs a normal rclcpp node against rmw_zenoh_rs. Nothing exotic: create a subscription, add the node to an executor, spin.

auto node = std::make_shared<rclcpp::Node>("listener");
auto sub  = node->create_subscription<std_msgs::msg::String>(
    "chatter", 10, [](std_msgs::msg::String::SharedPtr) {});

rclcpp::executors::SingleThreadedExecutor exec;
exec.add_node(node);      // installs the on_new_message callback
exec.spin();

add_node / remove_node are how rclcpp executors attach and detach, and attaching is what installs the on_new_message callback. This is the entire surface.

What happens

The node hangs at startup, intermittently. No error, no crash — it simply never spins.

The trigger is the ordinary startup race, not an exotic one: if any messages arrived on the topic before the executor attached, rmw_subscription_set_on_new_message_callback replays the backlog — invoking the freshly-installed executor callback while holding two mutexes. A callback that re-enters the rmw API for that same entity then blocks on a lock its own thread already holds. rclcpp re-registers callbacks on add_node/remove_node, so this is reachable, and it needs no race of its own once the backlog exists.

Four more sites have the same shape on the per-message delivery path, which runs on the zenoh delivery thread for every incoming message.

Root cause

std::sync::Mutex is not reentrant. Five sites in crates/rmw-zenoh-rs/ invoked an rclcpp executor callback with one or two guards still live:

Site	Guards held across the call	Severity
rmw_subscription_set_on_new_message_callback	callback + unread_count	highest — hit by the ordinary startup backlog replay
subscription delivery notifier (rmw.rs)	callback + callback_user_data	high — hot path, every incoming message
service delivery notifier (rmw.rs)	callback + callback_user_data	high
client delivery notifier (service.rs, send_request)	callback + callback_user_data	high
rmw_{service,client}_set_on_new_*_callback	unread_count	lower — callback is not also held, so the zenoh thread is serialised rather than deadlocked, unless the callback re-enters this same function

The shape, from the delivery notifier:

if let Ok(cb) = callback_holder_clone.lock() {
    if let Some(callback_fn) = *cb {
        if let Ok(user_data_usize) = user_data_holder_clone.lock() {
            unsafe { callback_fn(user_data_ptr, 1); }   // rclcpp executor, under two guards

The three-mutex layout (callback, callback_user_data, unread_count) is itself part of the problem: notification had to hold two guards at once to read a callback and its user-data together, and correctness rested on every site agreeing on a lock order.

Minimal repro

The crate had no test coverage for this behaviour at all, so the repro is the unit tests added with the fix. In Rust, without ROS:

let slot = ExecCallback::new("site");
slot.item_arrived();                       // backlog of 1, no callback yet
let slot_c = slot.clone();
slot.set(move |_, n| {
    // re-enter the same slot from inside its own dispatch
    slot_c.item_arrived();                 // hangs on an unfixed build
}, user_data);

installing_a_callback_over_a_backlog_survives_reentry is exactly this, and it reproduces the highest-severity site directly. no_guard_is_live_when_the_callback_runs asserts the live-guard count is zero at the moment of dispatch, which catches the defect even where re-entry is not attempted.

How this was found

Not a user report, and — importantly — not found by the manual sweep that produced the other fixes in this series. That sweep found five instances across the core and missed these five entirely.

They were found by a mechanical pass: enumerate every lock acquisition site in the workspace, classify each guard's lifetime (bound to a let, a temporary in a larger expression, a for … in map.iter() loop, a match scrutinee), then look inside that lifetime for a call into user code — stored Box<dyn Fn>/Arc<dyn Fn> fields, generic F: Fn parameters, anything named callback/cb/handler/on_*/notify/trigger, and extern "C" fn pointers stored and later called. Plus a lock-order graph for ABBA inversions.

A cheaper mechanical option was tried first and does not work — clippy::significant_drop_in_scrutinee reports zero hits on this crate despite five genuine instances. Reasoning in #254, which owns that argument; the short version is visible in the snippet above: if let Ok(cb) = holder.lock() binds the guard, so it is never a scrutinee temporary.

Detector evidence, both directions

Eight unit tests ship with the fix. Baseline cargo test -p rmw-zenoh-rs --lib is 13 passed. Reverting only the guard-drop and keeping the tests fails four of them:

installing_a_callback_over_a_backlog_survives_reentry   deadline — re-entrant call did not return (5s)
delivery_notification_survives_reentry_into_itself      deadline — re-entrant call did not return (5s)
delivery_notification_survives_reentry_into_set         deadline — re-entrant call did not return (5s)
no_guard_is_live_when_the_callback_runs                 assertion `left == right` failed:
                                                        set() must dispatch the backlog with no guard live

The three re-entry tests carry a 5 s deadline rather than blocking indefinitely, so they fail rather than wedge the binary. The remaining four tests pass with the defect reintroduced — only the four above are detectors.

Fixed by

#262.