fix(rmw): stop invoking executor callbacks under a lock

[YuanYuYuan]

Description

Fixes #261

All five remaining callback-under-lock sites in rmw-zenoh-rs invoked an rclcpp executor callback with one or two mutex guards still live:

rmw_subscription_set_on_new_message_callback   callback + unread_count
subscription delivery notifier                 callback + callback_user_data
service delivery notifier                      callback + callback_user_data
client delivery notifier (send_request)        callback + callback_user_data
rmw_{service,client}_set_on_new_*_callback     unread_count

std::sync::Mutex is not reentrant, so a callback that re-enters the rmw API for the same entity blocks on a lock its own thread already holds. Installing and clearing on_new_message callbacks is how rclcpp executors attach and detach, so this is reachable, and it needs no race: the worst of the five is hit by the ordinary startup path where messages arrive before the executor attaches and the backlog is replayed under two guards.

Depends on pr/1-reentrancy-tripwire (for TrackedMutex and invoke_user_callback!) and on pr/4b-event-graph-reentrancy (which also edits rmw.rs, for the event-status API change). Branched off pr/4b accordingly.

Stacked PR — this PR's base is pr/4b-event-graph-reentrancy, not main. The diff therefore shows only this branch's own commit. Merge order is pr/1 -> pr/4b -> this one; GitHub retargets automatically as each parent lands.

Key Changes

• crates/rmw-zenoh-rs/src/exec_callback.rs (new) — one ExecCallback slot replaces the per-entity trio of mutexes. Both of its operations collect what they need under the lock, drop the guard, and only then call into user code.

  Collapsing three mutexes into one is part of the fix, not tidying. With three, 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. One slot removes the ordering obligation entirely.

• set waits for in-flight dispatches on other threads. Dropping the guard before dispatch is necessary but not sufficient: it also discards a lifetime guarantee the reference implementation provides on purpose. rmw_zenoh_cpp invokes the callback under event_mutex_ (DataCallbackManager::trigger_callback) and set_callback takes the same mutex, so once set_callback(nullptr) returns no callback is in flight and the caller may free what user_data pointed at.

  Without that, this sequence is a use-after-free: a delivery thread snapshots (callback, user_data) and drops the guard; set(None, ..) takes the lock, clears the slot and returns without waiting; the entity is destroyed and user_data freed; the delivery thread then calls through its stale snapshot.

  Restoring the C++ shape would reinstate the deadlock, so exclusion and lifetime are separated. Each dispatch registers the thread performing it, and set waits for registrations on other threads before returning. Scoping the wait that way is what keeps a callback re-entering set on its own thread from waiting on itself — the distinction that makes this different from the original bug. Registration happens while the state guard is still held, so every dispatch holding the outgoing pointer is visible to the wait.

• The slot uses hiroz::reentrancy::TrackedMutex and dispatches through invoke_user_callback!, so a reintroduction panics in debug with the site name instead of hanging. Neither the state guard nor the in-flight guard is held across user code; where both are taken the order is always state then in-flight.

• rmw.rs, service.rs, pubsub.rs, lib.rs — migrate the five sites to the slot.

What fails without this

This crate had no test coverage for this behaviour at all, which is the main reason these five survived earlier sweeps. Sixteen unit tests ship with the fix (cargo test -p rmw-zenoh-rs --lib: 16 passed, up from 5 on pr/4b).

Every mechanism was proven to be load-bearing by reverting it independently and confirming the corresponding test fails. All three reverts were run in a ROS-provisioned shell against this head.

Revert 1 — remove the in-flight wait from set (the use-after-free):

set_waits_for_a_dispatch_in_flight_on_another_thread   FAILED
    set() returned while a callback was still in flight on another thread —
    rclcpp is now free to destroy the entity and free the user_data that
    callback is still holding
test result: FAILED. 0 passed; 1 failed

That test pins a callback inside a dispatch on one thread, calls set(None, ..) on another, and asserts by ticket order that set returned strictly after the callback finished — not merely that it eventually returned.

Revert 2 — wait for all threads instead of only other threads (the deadlock the scoping avoids). The run never finishes; it is killed at a 600 s timeout (rc=124), because set registers its own backlog replay and then waits for it:

delivery_notification_survives_reentry_into_set          FAILED
installing_a_callback_over_a_backlog_survives_reentry    FAILED
set_does_not_wait_for_a_dispatch_on_its_own_thread       FAILED
installing_a_callback_drains_the_backlog                 has been running for over 60 seconds
no_guard_is_live_when_the_callback_runs                  has been running for over 60 seconds

Note the two that hang rather than fail: they have no deadline, and they are not re-entrancy tests — installing_a_callback_drains_the_backlog is ordinary backlog delivery. So an unconditional wait breaks the normal path too, not just the re-entrant one. That is why the wait is scoped to other threads rather than simply added.

Revert 3 — make DispatchToken::drop a no-op (the leak that would turn the fix into a hang):

an_unwind_through_a_dispatch_releases_its_registration   FAILED
    set() after an unwind mid-dispatch did not return within 5s, so it is blocked
test result: FAILED. 0 passed; 1 failed   (5.17s)

An unwind through a live dispatch must deregister, or the wait in set becomes unsatisfiable forever. Note the unwind that matters is Rust-side: the callbacks are extern "C", and a panic out of one aborts the process rather than unwinding, so a panicking executor callback never reaches any Drop. What can unwind is invoke_user_callback!'s own debug assertion, which fires before the call — an earlier version of this test panicked from the extern "C" callback and killed the test binary with rc=101 instead of asserting anything.

The original deadlock detectors, unchanged and still proven in both directions. Reverting only the guard-drop in exec_callback.rs fails four:

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 assert_completes deadline rather than wedging the binary, so a revert is safe to try under CI. no_guard_is_live_when_the_callback_runs fails immediately on the assertion instead, which is what lets it catch the defect even at sites where re-entry is not attempted.

cargo check -p rmw-zenoh-rs --all-targets and cargo clippy -p rmw-zenoh-rs --all-targets -- -D warnings are clean.

Breaking Changes

None to the rmw C ABI, and no signature changes.

One behavioural change worth stating: set can now block. It returns only once dispatches holding the outgoing user_data have finished, so it waits as long as an executor callback already running on another thread takes to return. rmw_zenoh_cpp has the same property — there the wait is on event_mutex_ — and it is the property that makes it safe to free user_data afterwards. A callback re-entering on its own thread never waits.

Residual, stated rather than hidden: two threads both dispatching this entity's callback and both re-entering set from inside it will wait on each other. Each is in a live dispatch that may still touch its user_data after set returns, so neither wait can safely be skipped. This is not a regression against the reference: rmw_zenoh_cpp cannot survive re-entrant set at all, because set_callback re-locks a non-recursive event_mutex_ on the thread already holding it — it deadlocks with one thread. The single-threaded case, which is the one rclcpp exercises when an executor detaches from inside a notification, is exactly what this PR makes work.

Notes

rmw-zenoh-rs cannot be built in the pure-Rust CI shell — its build script needs ROS headers (rcutils/strdup.h) — so this PR's gates were run in a ROS-provisioned shell. Worth knowing when reviewing: cargo clippy --workspace in the pure-Rust shell silently cannot reach this crate, which is part of why it accumulated five instances of a defect class the rest of the tree had been swept for.

Rebased onto pr/4b's current tip (0c274185, itself rebased onto pr/1's a70f7336). This branch's four commits replayed with no conflicts and its own diff is byte-identical before and after; check, clippy and all 16 tests were re-run green on the rebased head (262184da).

Because this PR is based on pr/4b rather than main, ci.yml does not run on it (3 checks, not 27), so none of the above has executed on a GitHub runner. Re-verify once the base moves to main — and note this branch is two levels deep, so it needs replaying after pr/4b is replayed.

Checklist

• Ran ./scripts/check-local.sh successfully
• Added/updated tests/documentation (if applicable)

[Copilot]

Pull request overview

Prevents RMW executor callbacks from running while mutex guards are held, avoiding re-entrant deadlocks.

Changes:

• Introduces a shared, lock-safe executor callback slot.
• Migrates subscription, service, and client notifications.
• Adds eight backlog and re-entrancy tests.

Reviewed changes

Copilot reviewed 5 out of 5 changed files in this pull request and generated no comments.

Show a summary per file

File	Description
src/exec_callback.rs	Implements and tests callback dispatch.
src/rmw.rs	Migrates callback registration and notification.
src/service.rs	Updates service and client storage.
src/pubsub.rs	Updates subscription storage.
src/lib.rs	Exports the new module.

---

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

[Copilot]

Pull request overview

Copilot reviewed 5 out of 5 changed files in this pull request and generated no new comments.