Graph and event callbacks are invoked with up to four locks held (plus an ABBA inversion)

[YuanYuYuan]

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

⚠️ This issue carries two defect classes, not one. Failure 1 is the re-entrancy defect #282 tracks. Failure 2 is a lock-order inversion (ABBA) — a race, needing no callback at all, and not covered by #282's property: no-callout-under-a-lock says nothing about two threads taking two locks in opposite orders.

Both are fixed by #260, so nothing is unaddressed. Flagged because the second one was overlooked when this issue was read for its re-entrancy content, and a design decision elsewhere was recorded against "no such inversion has been observed" — which this issue already contradicted. A bundled issue hides its own contents the way a bundled PR does.

What a user does

Registers a graph or endpoint event callback — the mechanism behind "tell me when a publisher appears on this topic" — and does something ordinary inside it: count matching publishers, register another callback, unregister itself.

let mgr = Arc::new(GraphEventManager::new());
let weak = Arc::downgrade(&mgr);

mgr.register_event_callback(gid, "/topic".into(), ZenohEventType::SubscriptionMatched,
    move |_change| {
        if let Some(m) = weak.upgrade() {
            // any of these hangs:
            //   m.register_event_callback(..)
            //   m.unregister_event_callback(..)
            //   graph.count_publishers("/topic")
        }
    })?;

Via the rmw layer this callback is user code handed straight to an rclcpp executor, so it is reached by every ordinary ROS 2 node without the author ever seeing hiroz.

What happens

Three distinct failures, spanning two defect classes:

• 1 and 3 are the re-entrancy defect Re-entrancy: user callbacks invoked while a lock is held #282 tracks — a callback invoked while a non-reentrant lock is held.
• 2 is a lock-order inversion, which that property does not cover: "no callout under a lock" says nothing about two threads taking two locks in opposite orders. See the warning at the top of this issue.

All three are fixed by #260.

1. Self-deadlock in the callback. Any callback that re-enters hiroz hangs forever on a lock its own thread already holds. The hot path in is the liveliness subscriber, so this fires on every liveliness token — i.e. every time any node anywhere on the domain creates or destroys an endpoint. At that moment the callback runs with three or four non-reentrant locks held.

2. An ABBA lock-order inversion that needs no callback at all. Two threads can deadlock each other with nothing user-supplied involved. The liveliness path takes GraphData and then event_callbacks; add_local_entity and the rmw-side callers reach event_callbacks without holding GraphData. Interleave them and both threads wait forever. This one is a race, unlike the rest of this series — it is timing-dependent and will present as an intermittent startup hang.

3. Event status. The same shape one layer up: EventsManager lives behind an Arc<Mutex<..>> shared with RmEventHandle, and update_event_status takes &mut self — so every caller necessarily holds that outer mutex, and the method fires the registered callback from inside it. The first thing an rclcpp event callback typically does is ask the handle that fired for the status behind it (rmw_take_event), which locks that same mutex on the same thread. Self-deadlock, no race required. All eight call sites in crates/rmw-zenoh-rs/ had this shape.

Root cause

crates/hiroz/src/event.rs:

• trigger_event_with_policy invoked callbacks under the event_callbacks guard.
• trigger_graph_change held both event_callbacks and entity_topics for the whole notification loop.
• the guard-condition sweep held trigger_guard_condition plus graph_guard_conditions.
• update_event_status released only the inner event_mutex; the outer Mutex<EventsManager> that RmEventHandle::take_event / set_callback / is_ready all take stayed held.

crates/hiroz/src/graph.rs, Graph::new_with_pattern — the liveliness subscriber held the GraphData mutex across the call into trigger_graph_change, which is both the third/fourth lock in failure 1 and the A-then-B leg of failure 2.

Minimal repro

Failure 1 (deterministic, no ROS needed):

let mgr = Arc::new(GraphEventManager::new());
let weak = Arc::downgrade(&mgr);
let once = AtomicUsize::new(0);

mgr.register_event_callback(gid(1), "/reentrant".into(), ZenohEventType::SubscriptionMatched,
    move |_| {
        if once.fetch_add(1, Ordering::SeqCst) == 0 {
            if let Some(m) = weak.upgrade() {
                let _ = m.register_event_callback(gid(9), "/other".into(),
                                                  ZenohEventType::SubscriptionMatched, |_| {});
            }
        }
    })?;

let appearing = Entity::Endpoint(EndpointEntity {
    id: 1, node: None, kind: EndpointKind::Publisher,
    topic: "/reentrant".into(), type_info: None, qos: Default::default() });

mgr.trigger_graph_change(&appearing, true, zid);   // hangs

Run on a worker thread with a wall-clock deadline. Variants: unregister instead of register; query the graph instead of mutating it (that one does not hang, it returns 0 publishers — a wrong answer rather than a stall, which is arguably worse).

How this was found

Not a user report. Found by an audit of hiroz's locking — specifically by walking each guard's lifetime and looking for a call into user code inside it, plus a lock-order graph for the ABBA. Each finding was then given the deadline-guarded repro above.

Detector evidence, both directions

With the fix reverted and the 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.

Fixed by

#260.