Skip to content

fix(rmw-zenoh-rs): release callback-mutex/user_data locks before calling out - #351

Open
YuanYuYuan wants to merge 2 commits into
mainfrom
feat/rmw-zenoh-rs-gil-deadlock-fix
Open

YuanYuYuan wants to merge 2 commits into
mainfrom
feat/rmw-zenoh-rs-gil-deadlock-fix

Conversation

@YuanYuYuan

@YuanYuYuan YuanYuYuan commented Sep 11, 2026

Copy link
Copy Markdown
Collaborator

Summary

Subscription/Service/Client each held callback and/or callback_user_data across a call into the registered notify/response callback — a self-deadlock if that callback re-enters (on the real rclpy path, the callback wants Python's GIL, and the setter runs on a GIL-holding thread).

The fix

Each notify/setter function now copies out what the call-out needs, releases every lock, and only then calls out — matching the pattern this codebase already uses elsewhere for the same shape:

// before
if let Ok(cb) = callback_holder.lock() {
    if let Some(callback_fn) = *cb {
        if let Ok(user_data) = user_data_holder.lock() {
            callback_fn(user_data, ...);   // both guards still held here
        }
    }
}

// after
let callback_fn = callback_holder.lock().map(|g| *g)?;
let user_data = user_data_holder.lock().map(|g| *g)?;
callback_fn(user_data, ...);   // both locks already released

Extracted the notify-closure and setter-body logic into free functions per entity, so each is directly unit-testable without a live zenoh session or the full RMW FFI chain.

Two locks per entity, not one

The first version of this fix released only the callback lock. That still deadlocked — callback_user_data is a second, independently-contended lock with the setter, holding it across the call-out reproduces the identical shape. Caught by actually running a regression test with a real embedded Python interpreter standing in for the GIL, not by inspection.

Before and after

today with this change
notify path calls out with the callback/user-data locks held calls out with both released
a callback that re-enters the setter deadlocks proceeds normally
unread_count (retroactive-notification tracking) unchanged unchanged

Verification

  • cargo fmt --check / cargo clippy --all-targets -- -D warnings: clean.
  • cargo test -p rmw-zenoh-rs --lib: all tests pass, including a regression test per entity (Subscription/Service/Client) with a real embedded Python interpreter contending for the same locks the real rclpy path would — real hang without this fix, clean pass (0.02s) with it.

Breaking changes

None. No public API changes; the fix is purely internal to the notify/setter call-out sites.

Related work

This is one of a small, independent set of public fixes for the same underlying defect shape (a lock held across a call into user/foreign code) across the zenoh/ROS 2 ecosystem: eclipse-zenoh/zenoh#2781 and YuanYuYuan/zenoh#1 (zenoh's own Rust core), and ros2/rmw_zenoh#1061 (the C++ RMW implementation, same shape, independently re-derived). Stacked on this PR: #352, an opt-in lock-tripwire regression guard over the locks this PR cleans up.

Subscription/Service/Client each held callback_holder and/or
user_data_holder across a call into the registered notify/response
callback -- a self-deadlock if that callback re-enters (on the real
rclpy path, the callback wants Python's GIL, and the setter runs on a
GIL-holding thread). Collect what the call-out needs, release every
lock, then call out.

Two locks per entity, not one: an earlier version of this fix only
released callback_holder and missed that user_data_holder is a second,
independently-contended AB-BA pair with the setter -- caught only by
actually running the regression test, not by inspection.

Extracts the notify-closure/setter-body logic into pub(crate) functions
so it's directly unit-testable, with a real embedded Python interpreter
standing in for rclpy's GIL. All three regression tests: real hang
without the fix, clean pass (0.02s) with it.
Independent review found a real divergence: the pre-fix subscription
setter nested the unread-check/call/reset inside callback_holder's own
lock, so a poisoned callback_holder meant nothing ran at all. The first
version of this fix computed the pending count independently of that
lock, so a poisoned callback_holder now reset unread and fired the
callback anyway -- a real, if narrow, behavior change. Restored the
nesting (service/client's setters never had this nesting in the
original, so they're unaffected).

Also strengthens subscription_setter_with_no_pending_messages_does_not_panic
to actually assert the callback never fired, not just that it compiled --
the test's own name claimed this but didn't check it.

Verified: fmt/clippy clean, all 11 tests pass, 5/5 clean runs (checked
for flakiness from the shared test statics running in cargo's default
parallel mode).
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant