Merge ec9fcf17ca into 9ca5af2c82

The example originally showed a certain deadlock, which was not as clear
as it could be. The new version shows intentionally racy code that may
result in a successful execution but may also deadlock.

Cargo.toml

@@ -36,3 +36,6 @@ backtraces = []
 # Feature names do not match crate names pending namespaced features.
 lockapi = ["lock_api"]
 parkinglot = ["parking_lot", "lockapi"]
+
+[build-dependencies]
+autocfg = "1.4.0"

README.md

@@ -56,10 +56,10 @@ introduce a cyclic dependency between your locks, the operation panics instead.
 immediately notice the cyclic dependency rather than be eventually surprised by it in production.
 
 Mutex tracing is efficient, but it is not completely overhead-free. If you cannot spare the
-performance penalty in your production environment, this library also offers debug-only tracing.
-`DebugMutex`, also found in the `stdsync` module, is a type alias that evaluates to `TracingMutex`
-when debug assertions are enabled, and to `Mutex` when they are not. Similar helper types are
-available for other synchronization primitives.
+performance penalty in your production environment, this library also offers debug-only tracing. The
+type aliases in `tracing_mutex::stdsync` correspond to tracing primitives from
+`tracing_mutex::stdsync::tracing` when debug assertions are enabled, and to primitives from
+`std::sync::Mutex` when they are not. A similar structure exists for other 
 
 The minimum supported Rust version is 1.70. Increasing this is not considered a breaking change, but
 will be avoided within semver-compatible releases if possible.
@@ -68,6 +68,7 @@ will be avoided within semver-compatible releases if possible.
 
 - Dependency-tracking wrappers for all locking primitives
 - Optional opt-out for release mode code
+- Optional backtrace capture to aid with reproducing cyclic mutex chains
 - Support for primitives from:
   - `std::sync`
   - `parking_lot`
@@ -76,7 +77,6 @@ will be avoided within semver-compatible releases if possible.
 ## Future improvements
 
 - Improve performance in lock tracing
-- Optional logging to make debugging easier
 - Better and configurable error handling when detecting cyclic dependencies
 - Support for other locking libraries
 - Support for async locking libraries

build.rs

@@ -0,0 +1,10 @@
+use autocfg::AutoCfg;
+
+fn main() {
+    // To avoid bumping MSRV unnecessarily, we can sniff certain features. Reevaluate this on major
+    // releases.
+    let ac = AutoCfg::new().unwrap();
+    ac.emit_has_path("std::sync::LazyLock");
+
+    autocfg::rerun_path("build.rs");
+}

examples/mutex_cycle.rs

@@ -1,26 +1,62 @@
 //! Show what a crash looks like
 //!
-//! This shows what a traceback of a cycle detection looks like. It is expected to crash.
+//! This shows what a traceback of a cycle detection looks like. It is expected to crash when run in
+//! debug mode, because it might deadlock. In release mode, no tracing is used and the program may
+//! do any of the following:
+//!
+//! - Return a random valuation of `a`, `b`, and `c`. The implementation has a race-condition by
+//!   design. I have observed (4, 3, 6), but also (6, 3, 5).
+//! - Deadlock forever.
+//!
+//! One can increase the SLEEP_TIME constant to increase the likelihood of a deadlock to occur. On
+//! my machine, 1ns of sleep time gives about a 50/50 chance of the program deadlocking.
+use std::thread;
+use std::time::Duration;
+
 use tracing_mutex::stdsync::Mutex;
 
 fn main() {
-    let a = Mutex::new(());
-    let b = Mutex::new(());
-    let c = Mutex::new(());
+    let a = Mutex::new(1);
+    let b = Mutex::new(2);
+    let c = Mutex::new(3);
 
-    // Create an edge from a to b
-    {
-        let _a = a.lock();
-        let _b = b.lock();
-    }
+    // Increase this time to increase the likelihood of a deadlock.
+    const SLEEP_TIME: Duration = Duration::from_nanos(1);
 
-    // Create an edge from b to c
-    {
-        let _b = b.lock();
-        let _c = c.lock();
-    }
+    // Depending on random CPU performance, this section may deadlock, or may return a result. With
+    // tracing enabled, the potential deadlock is always detected and a backtrace should be
+    // produced.
+    thread::scope(|s| {
+        // Create an edge from a to b
+        s.spawn(|| {
+            let a = a.lock().unwrap();
+            thread::sleep(SLEEP_TIME);
+            *b.lock().unwrap() += *a;
+        });
 
-    // Now crash by trying to add an edge from c to a
-    let _c = c.lock();
-    let _a = a.lock(); // This line will crash
+        // Create an edge from b to c
+        s.spawn(|| {
+            let b = b.lock().unwrap();
+            thread::sleep(SLEEP_TIME);
+            *c.lock().unwrap() += *b;
+        });
+
+        // Create an edge from c to a
+        //
+        // N.B. the program can crash on any of the three edges, as there is no guarantee which
+        // thread will execute first. Nevertheless, any one of them is guaranteed to panic with
+        // tracing enabled.
+        s.spawn(|| {
+            let c = c.lock().unwrap();
+            thread::sleep(SLEEP_TIME);
+            *a.lock().unwrap() += *c;
+        });
+    });
+
+    println!(
+        "{}, {}, {}",
+        a.into_inner().unwrap(),
+        b.into_inner().unwrap(),
+        c.into_inner().unwrap()
+    );
 }

src/lib.rs

@@ -193,6 +193,14 @@ impl MutexId {
             unreachable!("Tried to drop lock for mutex {:?} but it wasn't held", self)
         });
     }
+
+    /// Execute the given closure while the guard is held.
+    pub fn with_held<T>(&self, f: impl FnOnce() -> T) -> T {
+        // Note: we MUST construct the RAII guard, we cannot simply mark held + mark released, as
+        // f() may panic and corrupt our state.
+        let _guard = self.get_borrowed();
+        f()
+    }
 }
 
 impl Default for MutexId {

src/parkinglot.rs

@@ -138,16 +138,14 @@ pub mod tracing {
         /// This method will panic if `f` panics, poisoning this `Once`. In addition, this function
         /// panics when the lock acquisition order is determined to be inconsistent.
         pub fn call_once(&self, f: impl FnOnce()) {
-            let _borrow = self.id.get_borrowed();
-            self.inner.call_once(f);
+            self.id.with_held(|| self.inner.call_once(f));
         }
 
         /// Performs the given initialization routine once and only once.
         ///
         /// This method is identical to [`Once::call_once`] except it ignores poisoning.
         pub fn call_once_force(&self, f: impl FnOnce(OnceState)) {
-            let _borrow = self.id.get_borrowed();
-            self.inner.call_once_force(f);
+            self.id.with_held(|| self.inner.call_once_force(f));
         }
     }
 }

src/stdsync.rs

@@ -30,6 +30,12 @@ pub use tracing::{
     Condvar, Mutex, MutexGuard, Once, OnceLock, RwLock, RwLockReadGuard, RwLockWriteGuard,
 };
 
+#[cfg(all(has_std__sync__LazyLock, debug_assertions))]
+pub use tracing::LazyLock;
+
+#[cfg(all(has_std__sync__LazyLock, not(debug_assertions)))]
+pub use std::sync::LazyLock;
+
 /// Dependency tracing versions of [`std::sync`].
 pub mod tracing {
     use std::fmt;
@@ -47,6 +53,12 @@ pub mod tracing {
     use crate::BorrowedMutex;
     use crate::LazyMutexId;
 
+    #[cfg(has_std__sync__LazyLock)]
+    pub use lazy_lock::LazyLock;
+
+    #[cfg(has_std__sync__LazyLock)]
+    mod lazy_lock;
+
     /// Wrapper for [`std::sync::Mutex`].
     ///
     /// Refer to the [crate-level][`crate`] documentation for the differences between this struct and
@@ -458,8 +470,7 @@ pub mod tracing {
         where
             F: FnOnce(),
         {
-            let _guard = self.mutex_id.get_borrowed();
-            self.inner.call_once(f);
+            self.mutex_id.with_held(|| self.inner.call_once(f))
         }
 
         /// Performs the same operation as [`call_once`][Once::call_once] except it ignores
@@ -473,8 +484,7 @@ pub mod tracing {
         where
             F: FnOnce(&OnceState),
         {
-            let _guard = self.mutex_id.get_borrowed();
-            self.inner.call_once_force(f);
+            self.mutex_id.with_held(|| self.inner.call_once_force(f))
         }
 
         /// Returns true if some `call_once` has completed successfully.
@@ -548,9 +558,7 @@ pub mod tracing {
         /// As this method may block until initialization is complete, it participates in cycle
         /// detection.
         pub fn set(&self, value: T) -> Result<(), T> {
-            let _guard = self.id.get_borrowed();
-
-            self.inner.set(value)
+            self.id.with_held(|| self.inner.set(value))
         }
 
         /// Gets the contents of the cell, initializing it with `f` if the cell was empty.
@@ -560,8 +568,7 @@ pub mod tracing {
         where
             F: FnOnce() -> T,
         {
-            let _guard = self.id.get_borrowed();
-            self.inner.get_or_init(f)
+            self.id.with_held(|| self.inner.get_or_init(f))
         }
 
         /// Takes the value out of this `OnceLock`, moving it back to an uninitialized state.

src/stdsync/tracing/lazy_lock.rs

@@ -0,0 +1,113 @@
+//! Wrapper implementation for LazyLock
+//!
+//! This lives in a separate module as LazyLock would otherwise raise our MSRV to 1.80. Reevaluate
+//! this in the future.
+use std::fmt;
+use std::fmt::Debug;
+use std::ops::Deref;
+
+use crate::LazyMutexId;
+
+/// Wrapper for [`std::sync::LazyLock`]
+///
+/// This wrapper participates in cycle detection like all other primitives in this crate. It should
+/// only be possible to encounter cycles when acquiring mutexes in the initialisation function.
+///
+/// # Examples
+///
+/// ```
+/// use tracing_mutex::stdsync::tracing::LazyLock;
+///
+/// static LOCK: LazyLock<i32> = LazyLock::new(|| {
+///     println!("Hello, world!");
+///     42
+/// });
+///
+/// // This should print "Hello, world!"
+/// println!("{}", *LOCK);
+/// // This should not.
+/// println!("{}", *LOCK);
+/// ```
+pub struct LazyLock<T, F = fn() -> T> {
+    inner: std::sync::LazyLock<T, F>,
+    id: LazyMutexId,
+}
+
+impl<T, F: FnOnce() -> T> LazyLock<T, F> {
+    /// Creates a new lazy value with the given initializing function.
+    pub const fn new(f: F) -> LazyLock<T, F> {
+        Self {
+            id: LazyMutexId::new(),
+            inner: std::sync::LazyLock::new(f),
+        }
+    }
+
+    /// Force this lazy lock to be evaluated.
+    ///
+    /// This is equivalent to dereferencing, but is more explicit.
+    pub fn force(this: &LazyLock<T, F>) -> &T {
+        &*this
+    }
+}
+
+impl<T, F: FnOnce() -> T> Deref for LazyLock<T, F> {
+    type Target = T;
+
+    fn deref(&self) -> &Self::Target {
+        self.id.with_held(|| &*self.inner)
+    }
+}
+
+impl<T: Default> Default for LazyLock<T> {
+    /// Return a `LazyLock` that is initialized through [`Default`].
+    fn default() -> Self {
+        Self::new(Default::default)
+    }
+}
+
+impl<T: Debug, F> Debug for LazyLock<T, F> {
+    fn fmt(&self, f: &mut fmt::Formatter<'_>) -> fmt::Result {
+        // Cannot implement this ourselves because the get() used is nightly, so delegate.
+        self.inner.fmt(f)
+    }
+}
+
+#[cfg(test)]
+mod tests {
+    use crate::stdsync::Mutex;
+
+    use super::*;
+
+    #[test]
+    fn test_only_init_once() {
+        let mut init_counter = 0;
+
+        let lock = LazyLock::new(|| {
+            init_counter += 1;
+            42
+        });
+
+        assert_eq!(*lock, 42);
+        LazyLock::force(&lock);
+
+        // Ensure we can access the init counter
+        drop(lock);
+
+        assert_eq!(init_counter, 1);
+    }
+
+    #[test]
+    #[should_panic(expected = "Found cycle")]
+    fn test_panic_with_cycle() {
+        let mutex = Mutex::new(());
+
+        let lock = LazyLock::new(|| *mutex.lock().unwrap());
+
+        // Establish the relation from lock to mutex
+        LazyLock::force(&lock);
+
+        // Now do it the other way around, which should crash
+        let _guard = mutex.lock().unwrap();
+        LazyLock::force(&lock);
+    }
+}