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

[YuanYuYuan]

Part of #282 — the defect class, the shared fix shape and the merge order are stated there.

Role in #282

Instance fix plus coverage, and the one that needed a different technique. Plain collect-release-call is not sufficient here; see What this PR does.

Relationship	PR / issue	Status
Consumes hiroz::reentrancy::TrackedMutex and invoke_user_callback!	#255	Merged — the dependency is already satisfied on main
Shares one file, crates/rmw-zenoh-rs/src/rmw.rs	#260	Open — a weak ordering. Rebase after whichever lands second

Base is main. No stacking.

Issue

Fixes #261. Six sites in rmw-zenoh-rs invoked an rclcpp executor callback with one or two mutex guards still live.

Site	Guards held across the call
subscription delivery notifier (rmw.rs)	callback + callback_user_data
service delivery notifier (rmw.rs)	callback + callback_user_data
client delivery notifier (ClientImpl::send_request, service.rs)	callback + callback_user_data
rmw_subscription_set_on_new_message_callback	callback + unread_count
rmw_service_set_on_new_request_callback	unread_count
rmw_client_set_on_new_response_callback	unread_count

std::sync::Mutex is not reentrant. Installing and clearing on_new_message callbacks is how rclcpp executors attach and detach, so a callback that re-enters the rmw API for the same entity blocks on a lock its own thread already holds. That is a deterministic hang, not a race.

The ordinary startup path reaches the worst of the six: messages arriving before the executor attaches leave a backlog, and the install path replays that backlog while holding two guards.

What this PR does

Adds crates/rmw-zenoh-rs/src/exec_callback.rs — one ExecCallback type shared by subscriptions, services and clients — and rewrites all six sites to use it. Collapsing three independent mutexes per entity into one is part of the fix: with three, "notify" 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.

Dropping the guard alone is not enough, and that is what makes this instance different. In rmw_zenoh_cpp, DataCallbackManager::trigger_callback dispatches under event_mutex_ (rmw_zenoh_cpp/src/detail/event.cpp:93-101) and set_callback takes the same mutex (event.cpp:73-89). That is not only exclusion — it is a lifetime guarantee: once set_callback(nullptr) returns, no callback is in flight, so the caller may free what user_data pointed at. Collect-then-dispatch discards that guarantee:

sequenceDiagram
    participant D as delivery thread
    participant E as executor thread
    D->>D: notify_one — snapshot (callback, user_data), drop guard
    E->>E: set(None) — take lock, clear slot, return
    E->>E: destroy entity, free user_data
    D->>D: call callback with the stale snapshot — use-after-free

Loading

So exclusion and the lifetime guarantee are separated:

• Each dispatch registers the thread performing it, while the state guard is still held, so set cannot conclude that no callback is in flight.
• set swaps the slot, drops the guard, then waits for registered dispatches on other threads only.

The thread distinction is the whole point. A callback re-entering set on its own thread must not wait — waiting for itself is the original deadlock. A set on an unrelated thread must wait, because it is the one about to free the pointer. That makes the fix caller-independent rather than correct-only-if-the-caller-is-a-different-thread.

A DispatchToken deregisters on scope exit including on unwind. Without that, an unwind through a live dispatch would leave the registration behind and every later set would block forever — trading a use-after-free for a permanent hang.

The state mutex is a TrackedMutex and both dispatch points go through invoke_user_callback!, so a reintroduction of the defect panics with the site name in debug builds and compiles to nothing in release.

Evidence

Claim	How it was established
26 CI checks pass, none failing, on cfaa2725	Measured — the PR's own check rollup at the current head
11 tests added, all in exec_callback.rs	Counted in the diff
rmw_zenoh_cpp dispatches and installs under the same non-recursive std::mutex	Read — event.cpp:73-101, event.hpp:91

Tests, by the property each one pins:

Test	Property
installing_a_callback_over_a_backlog_survives_reentry	The startup-race site: replaying a backlog survives a callback that clears itself
delivery_notification_survives_reentry_into_set	Delivery notifier survives re-entry into the install path
delivery_notification_survives_reentry_into_itself	Delivery notifier survives re-entry into itself
no_guard_is_live_when_the_callback_runs	live_guards() == 0 at both dispatch points — the tripwire is not passing vacuously
set_waits_for_a_dispatch_in_flight_on_another_thread	The lifetime guarantee: set returns strictly after the in-flight callback
set_does_not_wait_for_a_dispatch_on_its_own_thread	The wait is scoped to other threads, so it does not reinstate the deadlock
an_unwind_through_a_dispatch_releases_its_registration	DispatchToken::drop runs on unwind
4 further tests	Pre-fix semantics preserved: unread counting, backlog drain, no spurious dispatch, clearing

Each deadlock detector runs its body on a worker thread and panics after 5 s. The timeout message names all three causes the suite distinguishes, so a failure does not misattribute itself. The detectors are written to fail against the pre-fix shape; that is established by reading them, not by executing a reverted tree while preparing this description.

Breaking changes

None to the rmw C ABI. No extern "C" signature changes.

Change	Who is affected	Before → after
SubscriptionImpl, ServiceImpl, ClientImpl lose the callback, callback_user_data and unread_count fields	Rust code inside the workspace touching those structs; no in-tree users remain	Three Arc<Mutex<..>> fields → one exec_callback: ExecCallback
set can now block	Callers of the three rmw_*_set_on_new_*_callback entry points	Returned immediately → returns once dispatches holding the outgoing user_data have finished

⚠️ The blocking change is deliberate and is what makes freeing user_data afterwards safe. set waits as long as an executor callback already running on another thread takes to return. rmw_zenoh_cpp has the same property, where the wait is on event_mutex_ instead. A callback re-entering on its own thread never waits.

⚠️ Two open defects — do not merge yet

Both are in exec_callback.rs and both concern the same mechanism.

1. set can deadlock against a single re-entering callback

set registers its replay token at :282 while holding the state guard, releases the guard at :287, and reaches wait_for_other_dispatches() at :294 with that token still live. The wait skips only the current thread (:204-211).

State: callback = None, unread > 0 — the ordinary startup backlog this PR exists for.

step	thread	action	line
1	A (executor)	set(Some(cb)) swaps the slot, matches Some(_) if unread > 0, pushes token(A), releases the guard	:281-283, :287
2	B (delivery)	notify_one() reads callback == Some, pushes token(B), releases, invokes the callback	:223-228, :244
3	A	waits; the predicate matches token(B) → blocks with token(A) live	:294, :208
4	B's callback	re-enters set(None, null), hits _ => None, blocks on token(A)	:285, :294

Neither can proceed: token(A) drops only after A's wait returns, token(B) only after B's callback returns.

Candidate fixes: evaluate the wait before registering the replay token, or exclude not-yet-dispatching replay tokens from the predicate at :208.

2. The documented residual is narrower than the real one

:77-78 states the deadlock needs "two threads mutually re-entering", and :78-80 names single-threaded re-entry as "the case this type makes work". The interleaving above needs exactly one re-entering thread — thread A is an ordinary install-over-a-backlog and never runs user code before blocking.

So a reader who trusts the documentation concludes a reachable deadlock is safe. The InFlight doc at :116-120 ("one entry per active dispatch") is also false for token(A), which names a dispatch that has not started.

Both need fixing, or the residual paragraph needs to state the real bound.

Coverage this does not have

behaviour	detector
the five rmw wiring sites	none — reverting pubsub.rs and the two subscription hunks of rmw.rs to main reintroduces #261's highest-severity site verbatim, and all 16 tests still pass
defect 1 above	none — no test puts two threads in set/dispatch with a backlog present
notify_all() in DispatchToken::drop	wrong failure mode — deleting it hangs the test binary; the joins have no timeout, and assert_completes is not applied to them
guard dropped before dispatch	vacuous under --release — the guard counter is a literal 0 there and the macro compiles to nothing
set_does_not_wait_for_a_dispatch_on_its_own_thread	byte-identical to delivery_notification_survives_reentry_into_set apart from the label; no revert separates them

Residual deadlock, 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, 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: set_callback re-locks a non-recursive event_mutex_ on the thread that already holds it, so it deadlocks with one thread. This deadlocks only with two threads mutually re-entering. The single-threaded case — what rclcpp exercises when an executor detaches from inside a notification — is the case this PR makes work.

No integration-level test. The eleven tests drive ExecCallback directly. Nothing here exercises a real rclcpp executor attaching to a live subscription.

Checklist

• Tests added — 11, covering the deadlock, the use-after-free, the unwind path, and the preserved pre-fix semantics
• Module-level documentation states the residual limitation and the register-before-release invariant
• ./scripts/check-local.sh — not re-run against cfaa2725; the 26 green CI checks on that commit are the current evidence

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