diff --git a/crates/rmw-zenoh-rs/Cargo.toml b/crates/rmw-zenoh-rs/Cargo.toml index 1e6667ac..340d3b6e 100644 --- a/crates/rmw-zenoh-rs/Cargo.toml +++ b/crates/rmw-zenoh-rs/Cargo.toml @@ -36,6 +36,7 @@ hiroz = { workspace = true, features = ["rmw"] } hiroz-protocol = { workspace = true } hiroz-schema = { workspace = true } strum = { workspace = true, features = ["derive"] } +lock-tripwire = { git = "https://github.com/YuanYuYuan/lock-tripwire.git", tag = "v0.1.1", optional = true } [lib] name = "rmw_zenoh_rs" @@ -58,5 +59,16 @@ test-all = ["test-core", "test-msgs"] # Legacy compatibility test-rmw = ["test-all"] +# Wires lock-tripwire into the callback-holder/user_data Mutex fields +# (Subscription/Service/Client). Off by default -- no change to shipped +# behavior. The plain fix already removes the deadlock this guards +# against; this is a regression guard, not a live fix -- it fires only if +# a future change reintroduces holding one of these locks across a +# call-out. Live in cargo test / a plain debug build; a no-op in a plain +# --release build unless lock-tripwire's own force-checks feature (or its +# per-package profile override) is also enabled -- see +# tripwire_compat.rs's module docs. +lock-tripwire-guard = ["dep:lock-tripwire"] + [dev-dependencies] pyo3 = { version = "0.22", features = ["auto-initialize"] } diff --git a/crates/rmw-zenoh-rs/src/lib.rs b/crates/rmw-zenoh-rs/src/lib.rs index 6c754e1a..a62aa739 100644 --- a/crates/rmw-zenoh-rs/src/lib.rs +++ b/crates/rmw-zenoh-rs/src/lib.rs @@ -23,6 +23,7 @@ pub mod ros; pub mod service; #[macro_use] pub mod traits; +pub mod tripwire_compat; pub mod type_support; pub mod utils; pub mod wait_set; diff --git a/crates/rmw-zenoh-rs/src/pubsub.rs b/crates/rmw-zenoh-rs/src/pubsub.rs index fb522a10..bf54eda9 100644 --- a/crates/rmw-zenoh-rs/src/pubsub.rs +++ b/crates/rmw-zenoh-rs/src/pubsub.rs @@ -83,17 +83,22 @@ impl PublisherImpl { pub(crate) fn build_subscription_notify_callback( notifier: std::sync::Arc, callback_holder: std::sync::Arc< - std::sync::Mutex, + crate::tripwire_compat::GuardedMutex, >, - user_data_holder: std::sync::Arc>, + user_data_holder: std::sync::Arc>, unread_count_holder: std::sync::Arc>, ) -> impl Fn() + Send + Sync + 'static { + use crate::tripwire_compat::LockGuarded; move || { notifier.notify_all(); - // The `.lock()` temporary is dropped at the end of this statement -- - // released before any call-out below, unlike a `match`/`if let` - // scrutinee, which would extend it across the whole arm. - let Ok(callback_fn) = callback_holder.lock().map(|g| *g) else { + // The `.lock_guarded()` temporary is dropped at the end of this + // statement -- released before any call-out below, unlike a + // `match`/`if let` scrutinee, which would extend it across the + // whole arm. + let Ok(callback_fn) = callback_holder + .lock_guarded("rmw_zenoh_rs::pubsub::notify_callback::callback") + .map(|g| *g) + else { return; }; match callback_fn { @@ -101,9 +106,17 @@ pub(crate) fn build_subscription_notify_callback( // Copied out and the lock released before the call-out -- // the setter locks this same mutex, so holding it here // would be a second AB-BA pair alongside `callback_holder`. - if let Ok(user_data_usize) = user_data_holder.lock().map(|g| *g) { + if let Ok(user_data_usize) = user_data_holder + .lock_guarded("rmw_zenoh_rs::pubsub::notify_callback::user_data") + .map(|g| *g) + { let user_data_ptr = user_data_usize as *const std::ffi::c_void; - unsafe { callback_fn(user_data_ptr, 1) }; // 1 new message + // Both locks above are already released -- this asserts + // that stays true even if a future change regresses it. + crate::guarded_call!( + "rmw_zenoh_rs::pubsub::notify_callback::call_out", + unsafe { callback_fn(user_data_ptr, 1) } // 1 new message + ); } } None => { @@ -122,13 +135,19 @@ pub(crate) fn build_subscription_notify_callback( /// stores the new callback, then calls out -- all three locks released /// before the call, none held during it. pub(crate) fn set_subscription_callback_core( - callback_holder: &std::sync::Mutex, - user_data_holder: &std::sync::Mutex, + callback_holder: &crate::tripwire_compat::GuardedMutex< + crate::ros::rmw_subscription_new_message_callback_t, + >, + user_data_holder: &crate::tripwire_compat::GuardedMutex, unread_count_holder: &std::sync::Mutex, callback: crate::ros::rmw_subscription_new_message_callback_t, user_data: *mut crate::c_void, ) { - if let Ok(mut ud) = user_data_holder.lock() { + use crate::tripwire_compat::LockGuarded; + + if let Ok(mut ud) = user_data_holder + .lock_guarded("rmw_zenoh_rs::rmw_subscription_set_on_new_message_callback::user_data") + { *ud = user_data as usize; } @@ -138,7 +157,9 @@ pub(crate) fn set_subscription_callback_core( // independently of this lock would make a poisoned callback_holder // silently reset progress and still fire the call, which is a real // (if narrow) behavior change from before, not just a refactor. - let Ok(mut cb) = callback_holder.lock() else { + let Ok(mut cb) = callback_holder + .lock_guarded("rmw_zenoh_rs::rmw_subscription_set_on_new_message_callback::callback") + else { return; }; let pending = if callback.is_some() { @@ -155,7 +176,10 @@ pub(crate) fn set_subscription_callback_core( if let (Some(callback_fn), Some(n)) = (callback, pending) { if n > 0 { - unsafe { callback_fn(user_data as *const std::ffi::c_void, n) }; + crate::guarded_call!( + "rmw_zenoh_rs::rmw_subscription_set_on_new_message_callback::call_out", + unsafe { callback_fn(user_data as *const std::ffi::c_void, n) } + ); } } } @@ -167,9 +191,10 @@ pub struct SubscriptionImpl { pub topic: CString, pub options: rmw_subscription_options_t, pub qos: rmw_qos_profile_t, - pub callback: - std::sync::Arc>, - pub callback_user_data: std::sync::Arc>, // Store pointer as usize for thread safety + pub callback: std::sync::Arc< + crate::tripwire_compat::GuardedMutex, + >, + pub callback_user_data: std::sync::Arc>, // Store pointer as usize for thread safety pub unread_count: std::sync::Arc>, // Track messages arrived before callback was set pub graph: std::sync::Arc, pub entity: hiroz::entity::EndpointEntity, @@ -860,11 +885,12 @@ pub extern "C" fn rmw_subscription_get_content_filter( #[cfg(test)] mod gil_deadlock_tests { + use std::sync::Arc; use std::sync::atomic::{AtomicBool, Ordering}; - use std::sync::{Arc, Mutex}; use std::time::{Duration, Instant}; use crate::c_void; + use crate::tripwire_compat::GuardedMutex as Mutex; // Raw C function pointers carry no captured state, so the handshake // between the notify thread and the setter thread has to live in @@ -915,7 +941,7 @@ mod gil_deadlock_tests { gil_wanting_callback as unsafe extern "C" fn(*const std::ffi::c_void, usize), ))); let user_data_holder = Arc::new(Mutex::new(0usize)); - let unread_count_holder = Arc::new(Mutex::new(0usize)); + let unread_count_holder = Arc::new(std::sync::Mutex::new(0usize)); let notify = super::build_subscription_notify_callback( notifier, @@ -982,7 +1008,7 @@ mod gil_deadlock_tests { let callback_holder: Arc> = Arc::new(Mutex::new(None)); let user_data_holder = Arc::new(Mutex::new(0usize)); - let unread_count_holder = Arc::new(Mutex::new(0usize)); + let unread_count_holder = Arc::new(std::sync::Mutex::new(0usize)); super::set_subscription_callback_core( &callback_holder, diff --git a/crates/rmw-zenoh-rs/src/rmw.rs b/crates/rmw-zenoh-rs/src/rmw.rs index c842b347..7a4d9655 100644 --- a/crates/rmw-zenoh-rs/src/rmw.rs +++ b/crates/rmw-zenoh-rs/src/rmw.rs @@ -537,9 +537,9 @@ pub extern "C" fn rmw_create_subscription( // Create shared callback and user_data holders that will be populated after SubscriptionImpl is created let callback_holder: std::sync::Arc< - std::sync::Mutex, - > = std::sync::Arc::new(std::sync::Mutex::new(None)); - let user_data_holder = std::sync::Arc::new(std::sync::Mutex::new(0usize)); // Store pointer as usize for thread safety + crate::tripwire_compat::GuardedMutex, + > = std::sync::Arc::new(crate::tripwire_compat::GuardedMutex::new(None)); + let user_data_holder = std::sync::Arc::new(crate::tripwire_compat::GuardedMutex::new(0usize)); // Store pointer as usize for thread safety let unread_count_holder = std::sync::Arc::new(std::sync::Mutex::new(0usize)); // Track unread messages // Create notification callback that will wake up wait sets and invoke user callback @@ -1015,9 +1015,9 @@ pub extern "C" fn rmw_create_client( // Create shared callback and user_data holders let callback_holder: std::sync::Arc< - std::sync::Mutex, - > = std::sync::Arc::new(std::sync::Mutex::new(None)); - let user_data_holder = std::sync::Arc::new(std::sync::Mutex::new(0usize)); + crate::tripwire_compat::GuardedMutex, + > = std::sync::Arc::new(crate::tripwire_compat::GuardedMutex::new(None)); + let user_data_holder = std::sync::Arc::new(crate::tripwire_compat::GuardedMutex::new(0usize)); // Build the client (notification callback will be set per-request in send_request) let zclient = match zclient_builder.build() { @@ -1211,9 +1211,9 @@ pub extern "C" fn rmw_create_service( // Create shared callback and user_data holders that will be populated after ServiceImpl is created let callback_holder: std::sync::Arc< - std::sync::Mutex, - > = std::sync::Arc::new(std::sync::Mutex::new(None)); - let user_data_holder = std::sync::Arc::new(std::sync::Mutex::new(0usize)); // Store pointer as usize for thread safety + crate::tripwire_compat::GuardedMutex, + > = std::sync::Arc::new(crate::tripwire_compat::GuardedMutex::new(None)); + let user_data_holder = std::sync::Arc::new(crate::tripwire_compat::GuardedMutex::new(0usize)); // Store pointer as usize for thread safety let unread_count_holder = std::sync::Arc::new(std::sync::Mutex::new(0usize)); // Track unread requests // Create notification callback that will wake up wait sets and invoke user callback diff --git a/crates/rmw-zenoh-rs/src/service.rs b/crates/rmw-zenoh-rs/src/service.rs index 6657698c..1d293f73 100644 --- a/crates/rmw-zenoh-rs/src/service.rs +++ b/crates/rmw-zenoh-rs/src/service.rs @@ -15,8 +15,9 @@ pub struct ClientImpl { pub options: rmw_client_options_t, pub request_ts: crate::type_support::ServiceTypeSupport, pub response_ts: crate::type_support::ServiceTypeSupport, - pub callback: std::sync::Arc>, - pub callback_user_data: std::sync::Arc>, + pub callback: + std::sync::Arc>, + pub callback_user_data: std::sync::Arc>, pub notifier: std::sync::Arc, /// Tracks responses that arrived while no callback was set pub unread_count: std::sync::Arc>, @@ -29,13 +30,19 @@ pub struct ClientImpl { /// for why the lock must be released before the call-out. pub(crate) fn build_client_notify_callback( notifier: std::sync::Arc, - callback_holder: std::sync::Arc>, - user_data_holder: std::sync::Arc>, + callback_holder: std::sync::Arc< + crate::tripwire_compat::GuardedMutex, + >, + user_data_holder: std::sync::Arc>, unread_count_holder: std::sync::Arc>, ) -> impl Fn() + Send + Sync + 'static { + use crate::tripwire_compat::LockGuarded; move || { notifier.notify_all(); - let Ok(callback_fn) = callback_holder.lock().map(|g| *g) else { + let Ok(callback_fn) = callback_holder + .lock_guarded("rmw_zenoh_rs::service::ClientImpl::notify_callback::callback") + .map(|g| *g) + else { return; }; match callback_fn { @@ -43,9 +50,15 @@ pub(crate) fn build_client_notify_callback( // Copied out and the lock released before the call-out -- // the setter locks this same mutex, so holding it here // would be a second AB-BA pair alongside `callback_holder`. - if let Ok(user_data_usize) = user_data_holder.lock().map(|g| *g) { + if let Ok(user_data_usize) = user_data_holder + .lock_guarded("rmw_zenoh_rs::service::ClientImpl::notify_callback::user_data") + .map(|g| *g) + { let user_data_ptr = user_data_usize as *const std::ffi::c_void; - unsafe { callback_fn(user_data_ptr, 1) }; + crate::guarded_call!( + "rmw_zenoh_rs::service::ClientImpl::notify_callback::call_out", + unsafe { callback_fn(user_data_ptr, 1) } + ); } } None => { @@ -61,12 +74,14 @@ pub(crate) fn build_client_notify_callback( /// [`crate::pubsub::set_subscription_callback_core`] for the same /// collect/reset/store/call-out-last pattern. pub(crate) fn set_client_callback_core( - callback_holder: &Mutex, - user_data_holder: &Mutex, + callback_holder: &crate::tripwire_compat::GuardedMutex, + user_data_holder: &crate::tripwire_compat::GuardedMutex, unread_count_holder: &Mutex, callback: rmw_client_new_response_callback_t, user_data: *mut c_void, ) { + use crate::tripwire_compat::LockGuarded; + let pending = if callback.is_some() { unread_count_holder.lock().ok().map(|mut unread| { let n = *unread; @@ -77,10 +92,14 @@ pub(crate) fn set_client_callback_core( None }; - if let Ok(mut cb) = callback_holder.lock() { + if let Ok(mut cb) = callback_holder + .lock_guarded("rmw_zenoh_rs::rmw_client_set_on_new_response_callback::callback") + { *cb = callback; } - if let Ok(mut ud) = user_data_holder.lock() { + if let Ok(mut ud) = user_data_holder + .lock_guarded("rmw_zenoh_rs::rmw_client_set_on_new_response_callback::user_data") + { // Matches the pre-fix behavior exactly: clearing the callback also // zeroes the stored user_data, regardless of what was passed in. *ud = if callback.is_some() { @@ -96,7 +115,10 @@ pub(crate) fn set_client_callback_core( "[rmw_client_set_on_new_response_callback] Invoking callback retroactively for {} unread responses", n ); - unsafe { callback_fn(user_data as *const std::ffi::c_void, n) }; + crate::guarded_call!( + "rmw_zenoh_rs::rmw_client_set_on_new_response_callback::call_out", + unsafe { callback_fn(user_data as *const std::ffi::c_void, n) } + ); } } } @@ -225,8 +247,9 @@ pub struct ServiceImpl { pub request_ts: crate::type_support::ServiceTypeSupport, pub response_ts: crate::type_support::ServiceTypeSupport, pub qos: rmw_qos_profile_t, - pub callback: std::sync::Arc>, - pub callback_user_data: std::sync::Arc>, + pub callback: + std::sync::Arc>, + pub callback_user_data: std::sync::Arc>, /// Tracks requests that arrived while no callback was set pub unread_count: std::sync::Arc>, pub graph: std::sync::Arc, @@ -239,13 +262,19 @@ pub struct ServiceImpl { /// must be released before the call-out. pub(crate) fn build_service_notify_callback( notifier: std::sync::Arc, - callback_holder: std::sync::Arc>, - user_data_holder: std::sync::Arc>, + callback_holder: std::sync::Arc< + crate::tripwire_compat::GuardedMutex, + >, + user_data_holder: std::sync::Arc>, unread_count_holder: std::sync::Arc>, ) -> impl Fn() + Send + Sync + 'static { + use crate::tripwire_compat::LockGuarded; move || { notifier.notify_all(); - let Ok(callback_fn) = callback_holder.lock().map(|g| *g) else { + let Ok(callback_fn) = callback_holder + .lock_guarded("rmw_zenoh_rs::service::notify_callback::callback") + .map(|g| *g) + else { return; }; match callback_fn { @@ -253,9 +282,15 @@ pub(crate) fn build_service_notify_callback( // Copied out and the lock released before the call-out -- // the setter locks this same mutex, so holding it here // would be a second AB-BA pair alongside `callback_holder`. - if let Ok(user_data_usize) = user_data_holder.lock().map(|g| *g) { + if let Ok(user_data_usize) = user_data_holder + .lock_guarded("rmw_zenoh_rs::service::notify_callback::user_data") + .map(|g| *g) + { let user_data_ptr = user_data_usize as *const std::ffi::c_void; - unsafe { callback_fn(user_data_ptr, 1) }; // 1 new request + crate::guarded_call!( + "rmw_zenoh_rs::service::notify_callback::call_out", + unsafe { callback_fn(user_data_ptr, 1) } // 1 new request + ); } } None => { @@ -271,12 +306,14 @@ pub(crate) fn build_service_notify_callback( /// [`crate::pubsub::set_subscription_callback_core`] for the same /// collect/reset/store/call-out-last pattern. pub(crate) fn set_service_callback_core( - callback_holder: &Mutex, - user_data_holder: &Mutex, + callback_holder: &crate::tripwire_compat::GuardedMutex, + user_data_holder: &crate::tripwire_compat::GuardedMutex, unread_count_holder: &Mutex, callback: rmw_service_new_request_callback_t, user_data: *mut c_void, ) { + use crate::tripwire_compat::LockGuarded; + let pending = if callback.is_some() { unread_count_holder.lock().ok().map(|mut unread| { let n = *unread; @@ -287,10 +324,14 @@ pub(crate) fn set_service_callback_core( None }; - if let Ok(mut cb) = callback_holder.lock() { + if let Ok(mut cb) = callback_holder + .lock_guarded("rmw_zenoh_rs::rmw_service_set_on_new_request_callback::callback") + { *cb = callback; } - if let Ok(mut ud) = user_data_holder.lock() { + if let Ok(mut ud) = user_data_holder + .lock_guarded("rmw_zenoh_rs::rmw_service_set_on_new_request_callback::user_data") + { // Matches the pre-fix behavior exactly: clearing the callback also // zeroes the stored user_data, regardless of what was passed in. *ud = if callback.is_some() { @@ -306,7 +347,10 @@ pub(crate) fn set_service_callback_core( "[rmw_service_set_on_new_request_callback] Invoking callback retroactively for {} unread requests", n ); - unsafe { callback_fn(user_data as *const std::ffi::c_void, n) }; + crate::guarded_call!( + "rmw_zenoh_rs::rmw_service_set_on_new_request_callback::call_out", + unsafe { callback_fn(user_data as *const std::ffi::c_void, n) } + ); } } } @@ -587,8 +631,9 @@ pub extern "C" fn rmw_client_response_subscription_get_actual_qos( #[cfg(test)] mod service_gil_deadlock_tests { + use crate::tripwire_compat::GuardedMutex as Mutex; + use std::sync::Arc; use std::sync::atomic::{AtomicBool, Ordering}; - use std::sync::{Arc, Mutex}; use std::time::{Duration, Instant}; use crate::c_void; @@ -629,7 +674,7 @@ mod service_gil_deadlock_tests { Some(gil_wanting_callback as unsafe extern "C" fn(*const std::ffi::c_void, usize)), )); let user_data_holder = Arc::new(Mutex::new(0usize)); - let unread_count_holder = Arc::new(Mutex::new(0usize)); + let unread_count_holder = Arc::new(std::sync::Mutex::new(0usize)); let notify = super::build_service_notify_callback( notifier, @@ -672,8 +717,9 @@ mod service_gil_deadlock_tests { #[cfg(test)] mod client_gil_deadlock_tests { + use crate::tripwire_compat::GuardedMutex as Mutex; + use std::sync::Arc; use std::sync::atomic::{AtomicBool, Ordering}; - use std::sync::{Arc, Mutex}; use std::time::{Duration, Instant}; use crate::c_void; @@ -714,7 +760,7 @@ mod client_gil_deadlock_tests { Some(gil_wanting_callback as unsafe extern "C" fn(*const std::ffi::c_void, usize)), )); let user_data_holder = Arc::new(Mutex::new(0usize)); - let unread_count_holder = Arc::new(Mutex::new(0usize)); + let unread_count_holder = Arc::new(std::sync::Mutex::new(0usize)); let notify = super::build_client_notify_callback( notifier, diff --git a/crates/rmw-zenoh-rs/src/tripwire_compat.rs b/crates/rmw-zenoh-rs/src/tripwire_compat.rs new file mode 100644 index 00000000..b219ff32 --- /dev/null +++ b/crates/rmw-zenoh-rs/src/tripwire_compat.rs @@ -0,0 +1,77 @@ +//! A tiny compatibility shim, feature-gated by `lock-tripwire-guard`. +//! +//! `GuardedMutex` is `lock_tripwire::TrackedMutex` when the feature is +//! on and a plain `std::sync::Mutex` when it's off -- same `::new`, so no +//! call site needs a second, feature-gated construction path. `.lock_guarded +//! (SITE)` is the one method both sides implement, so a call site reads +//! identically either way. `guarded_call!` wraps a call-out that happens +//! while a `GuardedMutex` guard is held on the same thread; with the feature +//! on this is `lock_tripwire::invoke_user_callback!`, with it off it is the +//! call, unwrapped. +//! +//! The plain fix already removed the callback-mutex-vs-GIL deadlock this +//! crate's notify/setter functions used to have: no lock is held across a +//! call-out anymore. What this feature adds is a regression guard -- if a +//! future change ever reintroduces holding one of these locks across a +//! call-out, `guarded_call!` fires immediately and names the site, instead +//! of the change silently reintroducing a hang. +//! +//! **Scope, stated precisely rather than implied**: `lock-tripwire`'s own +//! tracking is gated on `debug_assertions` (see its crate docs), so this +//! guard is live in `cargo test` and any plain debug build, but compiles +//! out to nothing in a plain `--release` build even with this feature on -- +//! it does not, on its own, guard a release binary. To also catch a +//! regression in a release build, either enable `lock-tripwire`'s own +//! `force-checks` feature explicitly (a real, measured cost -- see that +//! crate's `OVERHEAD.md` before turning it on unconditionally here), or use +//! its per-package Cargo profile override +//! (`[profile.release.package.lock-tripwire] debug-assertions = true`) in +//! the top-level binary's own `Cargo.toml`. Deliberately not forwarded +//! automatically by `lock-tripwire-guard` -- that decision belongs to +//! whoever builds the release binary, not to this crate. + +#[cfg(feature = "lock-tripwire-guard")] +pub type GuardedMutex = lock_tripwire::TrackedMutex; +#[cfg(not(feature = "lock-tripwire-guard"))] +pub type GuardedMutex = std::sync::Mutex; + +#[cfg(feature = "lock-tripwire-guard")] +pub type GuardedMutexGuard<'a, T> = lock_tripwire::TrackedMutexGuard<'a, T>; +#[cfg(not(feature = "lock-tripwire-guard"))] +pub type GuardedMutexGuard<'a, T> = std::sync::MutexGuard<'a, T>; + +pub trait LockGuarded { + fn lock_guarded(&self, site: &'static str) -> std::sync::LockResult>; +} + +#[cfg(feature = "lock-tripwire-guard")] +impl LockGuarded for GuardedMutex { + #[inline] + fn lock_guarded(&self, site: &'static str) -> std::sync::LockResult> { + self.lock_at(site, lock_tripwire::LockKind::State) + } +} + +#[cfg(not(feature = "lock-tripwire-guard"))] +impl LockGuarded for GuardedMutex { + #[inline] + fn lock_guarded(&self, _site: &'static str) -> std::sync::LockResult> { + self.lock() + } +} + +#[cfg(feature = "lock-tripwire-guard")] +#[macro_export] +macro_rules! guarded_call { + ($site:expr, $call:expr) => { + ::lock_tripwire::invoke_user_callback!($site, $call) + }; +} + +#[cfg(not(feature = "lock-tripwire-guard"))] +#[macro_export] +macro_rules! guarded_call { + ($site:expr, $call:expr) => { + $call + }; +}