feat(rmw-zenoh-rs): add lock-tripwire as a regression guard - #352
Open
YuanYuYuan wants to merge 2 commits into
Open
YuanYuYuan wants to merge 2 commits into
YuanYuYuan wants to merge 2 commits into
Conversation
The plain fix already removes the callback-mutex/GIL AB-BA deadlock; this adds detection so a future regression that reintroduces holding callback_holder or callback_user_data across a call-out fails fast and by name instead of silently reintroducing a hang in production. Off by default -- no change to shipped behavior. Wraps both locks that matter to this hazard: callback_holder (the original scope) and callback_user_data (the second lock this crate's own plain fix found was also contended with the setter -- not covered by the original lock-tripwire-guard design, added here since it's the same hazard shape). unread_count stays untracked, matching the established, disclosed scope. Verified: fmt/clippy clean both feature states; all 11 tests pass (0.02-0.03s) both feature states, no regressions, no false positives.
Independent review found the doc comments implied this guards a plain release build, when in fact lock-tripwire's own tracking is gated on debug_assertions -- a release build silently checks nothing unless force-checks (or the per-package profile override) is also enabled. Not forwarding force-checks automatically: it has a real, measured cost (see lock-tripwire's own OVERHEAD.md), and that's a decision for whoever builds the release binary, not this crate. Corrected the docs to say so plainly instead of leaving the gap implicit.
This was referenced Sep 11, 2026
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Stacked on the plain fix in this repo (the previous PR). Adds an opt-in
lock-tripwire-guardCargo feature that wrapslock-tripwire— a small, debug-time tripwire that panics when a thread calls into opaque/user code while holding a lock it tracked — around the callback/user-data locks the plain fix just cleaned up.This is a regression guard, not a live fix: the previous PR already removes the deadlock unconditionally. If a future change ever reintroduces holding one of these locks across a call-out, this fires immediately and names the exact site, instead of silently reintroducing a hang.
Scope
Covers both locks the plain fix needed to release —
callbackandcallback_user_data— for all three entities (Subscription/Service/Client).unread_countstays untracked; nothing in this codebase holds it across a call-out.Off by default.
GuardedMutex<T>is a plainstd::sync::Mutex<T>when the feature is off, so there is no behavior change for anyone not opting in.Real, verified regression-catch proof
Built a throwaway local demo (not part of this diff): reintroduced holding the callback lock across the call-out in the subscription notify path, keeping the tripwire wiring in place.
timeout 20kills itScope of the guard, stated precisely
lock-tripwire's own tracking is gated ondebug_assertions, so this guard is live incargo testand any debug build, but is a no-op in a plain--releasebuild unlesslock-tripwire's ownforce-checksfeature (or its per-package Cargo profile override) is also enabled. Not forwarded automatically here — it has a real, measured runtime cost, and enabling it is a decision for whoever builds the release binary, documented in this crate's own doc comments.Verification
cargo fmt --check/cargo clippy --all-targets -- -D warnings, both feature states: clean.cargo test -p rmw-zenoh-rs --lib, both feature states: all tests pass (0.02–0.03s), no regressions, no false positives.lock-tripwireresolves from its public GitHub repository — verified for real, not assumed.Breaking changes
None. Opt-in feature, off by default.
Related work
Beyond the plain fix this stacks on (#351), sibling public fixes for the same underlying defect shape 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).