fix(event,graph): run event callbacks outside the locks they live under

[YuanYuYuan]

Description

Fixes #259

GraphEventManager invoked registered callbacks while holding the registries they are registered in, and EventsManager::update_event_status fired its callback from a &mut self method whose caller necessarily holds the shared outer mutex. The hot path in is the liveliness subscriber, which held the GraphData mutex across the call — so on every liveliness token these callbacks ran with three or four non-reentrant locks held, and they are user code the rmw layer hands straight to an rclcpp executor.

That nesting is also a lock-order inversion, not only a re-entrancy hazard: the liveliness path takes GraphData then event_callbacks, while add_local_entity and the rmw-side callers reach event_callbacks without GraphData, so two threads could interleave into a classic ABBA with no callback involved at all.

Depends on pr/1-reentrancy-tripwire, which supplies TrackedMutex and invoke_user_callback!. Branched off it accordingly.

Stacked PR — this PR's base is pr/1-reentrancy-tripwire, not main. The diff therefore shows only this branch's own commit. Merge pr/1 first; GitHub will retarget this one to main automatically when it does.

Key Changes

• crates/hiroz/src/event.rs — collect under the lock, drop every guard, then invoke, the way zenoh core does it in Session::resolve_put. EventCallback becomes Arc instead of Box so it can be cloned out cheaply.

• crates/hiroz/src/graph.rs — the liveliness subscriber scopes its GraphData guard so it is released before any trigger, matching what add_local_entity already did. This is what closes the ABBA leg.

• Event status needed a split, not a scope change. Collect-then-invoke cannot be done from inside a &mut self method, which does not own the guard and so cannot drop it. record_event_status_with_policy now records and returns the callback owed a notification without calling it, and two free functions, update_shared_event_status[_with_policy], take the Mutex<EventsManager> directly so they can drop the guard before invoking. Those are now the entry point for every holder of a shared manager. update_event_status[_with_policy] stay as thin wrappers for callers that own the manager outright (the unit tests), documented as such.

• Guard conditions become an owned trait object, because dropping the lock creates a lifetime problem the old design could not express. Once the registry lock is released before triggering, a registered raw *mut c_void can be freed by a concurrent rmw_destroy_node in the window between snapshot and call — the trigger would then write through dangling memory. So the GraphGuardConditionTrigger alias is removed and replaced by a GraphGuardCondition trait; the registry holds Arc<dyn GraphGuardCondition> and shared ownership keeps the target alive for the duration of any in-flight trigger. See Breaking Changes.

• crates/rmw-zenoh-rs/ — the rmw half of that redesign, which is most of this diff's surface outside hiroz:

  • New guard_condition.rs splits the triggerable state (GuardConditionState) from the C handle, behind an Arc so it can outlive rmw_destroy_guard_condition. It implements hiroz::event::GraphGuardCondition.
  • triggered becomes an AtomicBool: triggering no longer happens under any lock, so a graph-event thread can set it while rmw_wait reads it. GuardConditionImpl's methods take &self, not &mut self, for the same reason.
  • node.rs registers the shared state, not the C pointer.
  • The eight update_event_status* call sites in rmw.rs move to update_shared_event_status*.

• The four registries become TrackedMutex and both trigger paths dispatch through invoke_user_callback!, so a reintroduction panics in debug naming the site rather than hanging.

Behavioural notes from collect-then-invoke, all only observable to a callback that previously deadlocked:

• trigger_graph_change now snapshots the notify list, so a callback that registers or unregisters during a sweep no longer affects that same sweep.
• EventsManager::set_callback fires its backlog notification after installing the callback rather than before.
• Event ordering relative to graph mutation is preserved deliberately: on a liveliness PUT the entity is inserted before the event fires, on a DELETE the event fires before removal, exactly as before. Only the lock is no longer held across the gap, so the mutation and the notification are no longer atomic with respect to each other.

What fails without this

Detector proven in both directions. With the fix reverted and the five deadline-guarded tests kept, all five fail:

event_callback_unregistering_does_not_deadlock              30s deadline — deadlock
graph_change_callback_registering_does_not_deadlock         30s deadline — deadlock
graph_change_callback_querying_the_graph_does_not_deadlock  the re-entrant graph query returned 0 publishers
event_callback_taking_its_own_status_does_not_deadlock      30s deadline — deadlock
event_callback_reinstalling_itself_does_not_deadlock        30s deadline — deadlock

With the fix, all five pass. Note the third: it does not hang, it returns a wrong answer (0 publishers). That one would have shipped silently.

Which half of the fix each detector is sensitive to, checked by reverting the two files separately, because it is not obvious from the test names:

Detector	Needs event.rs	Needs graph.rs
event_callback_unregistering_does_not_deadlock	yes	—
graph_change_callback_registering_does_not_deadlock	yes	—
event_callback_taking_its_own_status_does_not_deadlock	yes	—
event_callback_reinstalling_itself_does_not_deadlock	yes	—
graph_change_callback_querying_the_graph_does_not_deadlock	—	yes

Reverting event.rs alone leaves the graph-query test green; it only goes red once the liveliness subscriber holds the GraphData guard across trigger_graph_change again. So graph.rs is not a supporting tidy-up — it is the sole cause of the wrong-answer failure, and it is the leg that closes the ABBA.

No regression: hiroz --lib 287/287, reentrant_graph_event 3/3, reentrant_event_status 2/2.

287, not the 286 an earlier revision of this description claimed. The extra one is this branch's own graph_guard_condition_registration_is_owned_by_the_manager, added by the guard-condition commit; the stale figure predated it.

Rebased onto pr/1's current tip (a70f7336); this branch's four commits replayed with no conflicts, and its own diff is byte-identical before and after. All figures above were measured on the rebased head (0c274185).

Coverage gap, stated plainly. rmw-zenoh-rs is not built by this repo's pure-Rust CI at all — its build script needs ROS headers the CI shell does not have — and because this PR is based on pr/1 rather than main, ci.yml does not run on it either (3 checks, not 27). The guard-condition redesign above is therefore the least CI-covered part of this change: it is the part with the most new code and the least automated evidence. cargo check -p rmw-zenoh-rs --all-targets and cargo clippy -p rmw-zenoh-rs --all-targets -- -D warnings were both run against this exact head (0c274185) in a ROS-provisioned shell and are clean. That is the only automated evidence for this half; the interop suites have not run on it. Re-verify once the base moves to main and the full matrix applies.

Breaking Changes

Yes — two public items change, and both are source-breaking.

1. pub type EventCallback goes Box<dyn Fn(i32) + Send + Sync> → Arc<dyn Fn(i32) + Send + Sync>. Callers update Box::new to Arc::new.

2. pub type GraphGuardConditionTrigger is removed, not converted. It was Box<dyn Fn(*mut c_void) + Send + Sync>; there is no Arc equivalent of it in this branch. In its place, register_graph_guard_condition / unregister_graph_guard_condition take Arc<dyn GraphGuardCondition> — a trait, not a closure. A caller that previously handed over a boxed closure must now define a type and implement GraphGuardCondition for it. This is a larger break than (1) and is not a mechanical substitution.

An earlier revision of this description claimed "None to the public API"; a later one said both aliases merely went Box → Arc. Both were wrong, in the direction of understating the break.

The redesign is load-bearing rather than stylistic. Box cannot be cloned, so it cannot be lifted out of a guard — which is the entire fix. And for guard conditions the closure form cannot express the ownership the fix requires: the registry must keep the target alive across a trigger it performs with no lock held, which needs shared ownership of the state, not a pointer captured in a closure.

update_event_status[_with_policy] remain, now documented as being for callers that own the manager outright.

Worth flagging for a follow-up rather than fixing here: EventsManager::update_event_status[_with_policy] is still pub and, taking &mut self, still necessarily runs the callback under whatever mutex the caller holds. Every in-tree caller was migrated to the shared-manager entry points, but the old one is re-armed for the next caller and is a candidate for #[deprecated] or removal.

Notes

cargo doc is red on main today for unrelated reasons; pr/0-rustdoc-links clears it. Run the reentrancy suites with --features ros-msgs,jazzy.

Checklist

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

[Copilot]

Pull request overview

Moves graph and event callbacks outside mutex guards to prevent re-entrant deadlocks and lock-order inversion.

Changes:

• Snapshots callbacks before invocation and introduces shared event-status helpers.
• Releases graph-data locks before graph notifications.
• Adds re-entrancy regression tests.

Reviewed changes

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

Show a summary per file

File	Description
crates/hiroz/src/event.rs	Refactors callback storage, locking, and dispatch.
crates/hiroz/src/graph.rs	Narrows graph-data guard lifetimes.
crates/rmw-zenoh-rs/src/rmw.rs	Uses shared event-status helpers.
crates/rmw-zenoh-rs/src/context.rs	Constructs the trigger callback with Arc.
crates/hiroz-tests/tests/reentrant_graph_event.rs	Tests graph callback re-entrancy.
crates/hiroz-tests/tests/reentrant_event_status.rs	Tests event-status callback re-entrancy.

---

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

[Copilot]

Pull request overview

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

Comments suppressed due to low confidence (6)

crates/hiroz/src/event.rs:81

• set_callback still invokes the backlog callback from a &mut self method. A public caller using the normal shared shape (manager.lock().unwrap().set_callback(...)) therefore keeps the outer Mutex<EventsManager> guard alive here, and a callback that calls take_event deterministically deadlocks. The install_callback split only avoids this through RmEventHandle; this wrapper needs the same shared-manager treatment, or it must be restricted/deprecated for exclusively owned managers. This invocation should also use the repository's invoke_user_callback! tripwire.

            callback(unread_count);

crates/hiroz/src/event.rs:457

• The shared-manager path correctly drops its mutex, but this is still invocation of the user callback and must use invoke_user_callback! under the rule in reentrancy.rs:43-49. Calling it directly leaves this newly introduced dispatch path outside the debug-time guard-lifetime detector.

    if let Some(callback) = callback {
        callback(change);
    }

crates/hiroz/src/event.rs:508

• This backlog notification is user code, so invoking it directly violates the explicit convention in reentrancy.rs:43-49 that every user-code call site uses invoke_user_callback!. Wrapping it preserves the lock-release fix while allowing debug builds to catch any tracked guard retained by future callers.

        if unread_count != 0 {
            callback(unread_count);
        }

crates/hiroz/src/event.rs:347

• GraphGuardCondition::trigger is an externally implemented callback (and the surrounding comment explicitly says it may re-enter the manager), but this dispatch bypasses invoke_user_callback!. Per reentrancy.rs:43-49, wrap this call so debug builds detect if any tracked hiroz guard remains live at this callback boundary.

        for gc in guard_conditions {
            gc.trigger();
        }

crates/hiroz/src/event.rs:203

• The PR's Breaking Changes section says GraphGuardConditionTrigger merely changes from Box to Arc, but this diff actually removes that public alias and set_guard_condition_trigger, introduces a new trait, and changes registration from a raw pointer to Arc<dyn GraphGuardCondition>. Downstream migration is therefore not the stated one-token Box::new → Arc::new update; update the compatibility design or document the full public API break and migration.

pub trait GraphGuardCondition: Send + Sync {
    /// Wake whatever is waiting on this guard condition.
    ///
    /// Called with no hiroz lock held, possibly concurrently, and possibly
    /// after the corresponding C handle has been destroyed.
    fn trigger(&self);

crates/hiroz/src/event.rs:123

• This is user callback dispatch, but it bypasses the mandatory re-entrancy tripwire. reentrancy.rs:43-49 requires every user-code invocation to go through invoke_user_callback!; without it, this public wrapper can silently invoke user code while another tracked hiroz guard is live.

This issue also appears in the following locations of the same file:

• line 345
• line 455
• line 506

        if let Some(callback) =
            self.record_event_status_with_policy(event_type, change, policy_kind)
        {
            callback(change);
        }

[Copilot]

Pull request overview

Copilot reviewed 8 out of 8 changed files in this pull request and generated 1 comment.

Comments suppressed due to low confidence (7)

crates/hiroz/src/event.rs:203

• This public trait replaces GraphGuardConditionTrigger and the diff also removes set_guard_condition_trigger and changes both registration signatures. The PR description instead says the alias merely changes from Box to Arc and lists only two alias-shape breaks. Please update the breaking-change and migration notes (or preserve compatibility) so downstream impact is accurately disclosed.

pub trait GraphGuardCondition: Send + Sync {
    /// Wake whatever is waiting on this guard condition.
    ///
    /// Called with no hiroz lock held, possibly concurrently, and possibly
    /// after the corresponding C handle has been destroyed.
    fn trigger(&self);

crates/hiroz/src/event.rs:122

• This callback invocation bypasses the mandatory invoke_user_callback! check documented in crates/hiroz/src/reentrancy.rs:43-49. The direct-manager path can still be reached while another tracked hiroz guard is live, so use the tripwire here as well.

            callback(change);

crates/hiroz/src/event.rs:346

• GraphGuardCondition is a public, externally implementable trait, so trigger() is also an invocation of user-supplied code. Per the invariant in crates/hiroz/src/reentrancy.rs:43-49, dispatch it through invoke_user_callback! just like the endpoint callbacks below.

            gc.trigger();

crates/hiroz/src/event.rs:456

• The shared-manager callback is user code but this new dispatch bypasses the invoke_user_callback! invariant in crates/hiroz/src/reentrancy.rs:43-49. Use the macro after dropping the manager guard so upstream tracked-lock regressions are detected.

        callback(change);

crates/hiroz/src/event.rs:507

• This backlog dispatch is another user callback site that skips the required tripwire (crates/hiroz/src/reentrancy.rs:43-49). Wrapping it preserves the lock-free behavior while detecting callers that still hold a tracked hiroz guard.

            callback(unread_count);

crates/hiroz/src/event.rs:245

• Releasing event_callbacks before inserting entity_topics makes registration observable in a partial state: trigger_event can already invoke the new callback, while a concurrent trigger_graph_change can miss it because its topic is not present yet. Keep the callbacks guard until the topic insertion completes (the trigger path already takes these locks in the same order) so registration has one atomic visibility point.

        {
            let mut callbacks = self.event_callbacks.lock().unwrap();
            let entity_callbacks = callbacks.entry(entity_gid).or_default();
            entity_callbacks.insert(event_type, Arc::new(callback));
        }

crates/hiroz/src/event.rs:81

• This invokes externally supplied callback code directly, bypassing the repository's required re-entrancy tripwire (crates/hiroz/src/reentrancy.rs:43-49). Route it through invoke_user_callback! so a callback reached while any tracked guard is live fails diagnostically instead of hanging.

This issue also appears in the following locations of the same file:

• line 122
• line 346
• line 456
• line 507

            callback(unread_count);