Show cycle backtraces when they happen

CHANGELOG.md

@@ -11,6 +11,7 @@ adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0.html).
 - The minimum supported Rust version is now defined as 1.70. Previously it was undefined.
 - Wrappers for `std::sync` primitives can now be `const` constructed.
 - Add support for `std::sync::OnceLock`
+- Added backtraces of mutex allocations to the cycle report. 
 
 ### Breaking
 

examples/mutex_cycle.rs

@@ -0,0 +1,26 @@
+//! Show what a crash looks like
+//!
+//! This shows what a traceback of a cycle detection looks like. It is expected to crash.
+use tracing_mutex::stdsync::Mutex;
+
+fn main() {
+    let a = Mutex::new(());
+    let b = Mutex::new(());
+    let c = Mutex::new(());
+
+    // Create an edge from a to b
+    {
+        let _a = a.lock();
+        let _b = b.lock();
+    }
+
+    // Create an edge from b to c
+    {
+        let _b = b.lock();
+        let _c = c.lock();
+    }
+
+    // Now crash by trying to add an edge from c to a
+    let _c = c.lock();
+    let _a = a.lock(); // This line will crash
+}

src/graph.rs

@@ -107,10 +107,18 @@ where
     ///
     /// Nodes, both from and to, are created as needed when creating new edges. If the new edge
     /// would introduce a cycle, the edge is rejected and `false` is returned.
-    pub(crate) fn add_edge(&mut self, x: V, y: V, e: impl FnOnce() -> E) -> bool {
+    ///
+    /// # Errors
+    ///
+    /// If the edge would introduce the cycle, the underlying graph is not modified and a list of
+    /// all the edge data in the would-be cycle is returned instead.
+    pub(crate) fn add_edge(&mut self, x: V, y: V, e: impl FnOnce() -> E) -> Result<(), Vec<E>>
+    where
+        E: Clone,
+    {
         if x == y {
             // self-edges are always considered cycles
-            return false;
+            return Err(Vec::new());
         }
 
         let (_, out_edges, ub) = self.add_node(x);
@@ -118,7 +126,7 @@ where
         match out_edges.entry(y) {
             Entry::Occupied(_) => {
                 // Edge already exists, nothing to be done
-                return true;
+                return Ok(());
             }
             Entry::Vacant(entry) => entry.insert(e()),
         };
@@ -133,7 +141,7 @@ where
             let mut delta_f = Vec::new();
             let mut delta_b = Vec::new();
 
-            if !self.dfs_f(&self.nodes[&y], ub, &mut visited, &mut delta_f) {
+            if let Err(cycle) = self.dfs_f(&self.nodes[&y], ub, &mut visited, &mut delta_f) {
                 // This edge introduces a cycle, so we want to reject it and remove it from the
                 // graph again to keep the "does not contain cycles" invariant.
 
@@ -143,7 +151,7 @@ where
                 self.nodes.get_mut(&x).map(|node| node.out_edges.remove(&y));
 
                 // No edge was added
-                return false;
+                return Err(cycle);
             }
 
             // No need to check as we should've found the cycle on the forward pass
@@ -155,7 +163,7 @@ where
             self.reorder(delta_f, delta_b);
         }
 
-        true
+        Ok(())
     }
 
     /// Forwards depth-first-search
@@ -165,25 +173,30 @@ where
         ub: Order,
         visited: &mut HashSet<V>,
         delta_f: &mut Vec<&'a Node<V, E>>,
-    ) -> bool {
+    ) -> Result<(), Vec<E>>
+    where
+        E: Clone,
+    {
         delta_f.push(n);
 
-        n.out_edges.keys().all(|w| {
+        for (w, e) in &n.out_edges {
             let node = &self.nodes[w];
             let ord = node.ord.get();
 
             if ord == ub {
                 // Found a cycle
-                false
+                return Err(vec![e.clone()]);
             } else if !visited.contains(w) && ord < ub {
                 // Need to check recursively
                 visited.insert(*w);
-                self.dfs_f(node, ub, visited, delta_f)
-            } else {
-                // Already seen this one or not interesting
-                true
+                if let Err(mut chain) = self.dfs_f(node, ub, visited, delta_f) {
+                    chain.push(e.clone());
+                    return Err(chain);
                 }
-        })
+            }
+        }
+
+        Ok(())
     }
 
     /// Backwards depth-first-search
@@ -260,7 +273,7 @@ mod tests {
         // Regression test for https://github.com/bertptrs/tracing-mutex/issues/7
         let mut graph = DiGraph::default();
 
-        assert!(!graph.add_edge(1, 1, nop));
+        assert!(graph.add_edge(1, 1, nop).is_err());
     }
 
     #[test]
@@ -268,16 +281,16 @@ mod tests {
         let mut graph = DiGraph::default();
 
         // Add some safe edges
-        assert!(graph.add_edge(0, 1, nop));
-        assert!(graph.add_edge(1, 2, nop));
-        assert!(graph.add_edge(2, 3, nop));
-        assert!(graph.add_edge(4, 2, nop));
+        assert!(graph.add_edge(0, 1, nop).is_ok());
+        assert!(graph.add_edge(1, 2, nop).is_ok());
+        assert!(graph.add_edge(2, 3, nop).is_ok());
+        assert!(graph.add_edge(4, 2, nop).is_ok());
 
         // Try to add an edge that introduces a cycle
-        assert!(!graph.add_edge(3, 1, nop));
+        assert!(graph.add_edge(3, 1, nop).is_err());
 
         // Add an edge that should reorder 0 to be after 4
-        assert!(graph.add_edge(4, 0, nop));
+        assert!(graph.add_edge(4, 0, nop).is_ok());
     }
 
     /// Fuzz the DiGraph implementation by adding a bunch of valid edges.
@@ -285,7 +298,7 @@ mod tests {
     /// This test generates all possible forward edges in a 100-node graph consisting of natural
     /// numbers, shuffles them, then adds them to the graph. This will always be a valid directed,
     /// acyclic graph because there is a trivial order (the natural numbers) but because the edges
-    /// are added in a random order the DiGraph will still occassionally need to reorder nodes.
+    /// are added in a random order the DiGraph will still occasionally need to reorder nodes.
     #[test]
     fn fuzz_digraph() {
         // Note: this fuzzer is quadratic in the number of nodes, so this cannot be too large or it
@@ -306,7 +319,7 @@ mod tests {
         let mut graph = DiGraph::default();
 
         for (x, y) in edges {
-            assert!(graph.add_edge(x, y, nop));
+            assert!(graph.add_edge(x, y, nop).is_ok());
         }
     }
 }

src/lib.rs

@@ -139,19 +139,28 @@ impl MutexId {
     ///
     /// This method panics if the new dependency would introduce a cycle.
     pub fn mark_held(&self) {
-        let creates_cycle = HELD_LOCKS.with(|locks| {
+        let opt_cycle = HELD_LOCKS.with(|locks| {
             if let Some(&previous) = locks.borrow().last() {
                 let mut graph = get_dependency_graph();
 
-                !graph.add_edge(previous, self.value(), MutexDep::capture)
+                graph
+                    .add_edge(previous, self.value(), MutexDep::capture)
+                    .err()
             } else {
-                false
+                None
             }
         });
 
-        if creates_cycle {
+        if let Some(cycle) = opt_cycle {
             // Panic without holding the lock to avoid needlessly poisoning it
-            panic!("Mutex order graph should not have cycles");
+            let mut message = String::from("Found cycle in mutex dependency graph:\n");
+
+            for entry in cycle {
+                use std::fmt::Write;
+                let _ = writeln!(message, "{entry}");
+            }
+
+            panic!("{message}");
         }
 
         HELD_LOCKS.with(|locks| locks.borrow_mut().push(self.value()));
@@ -270,6 +279,12 @@ impl MutexDep {
     }
 }
 
+impl fmt::Display for MutexDep {
+    fn fmt(&self, f: &mut fmt::Formatter<'_>) -> fmt::Result {
+        self.0.fmt(f)
+    }
+}
+
 /// Get a reference to the current dependency graph
 fn get_dependency_graph() -> impl DerefMut<Target = DiGraph<usize, MutexDep>> {
     static DEPENDENCY_GRAPH: OnceLock<Mutex<DiGraph<usize, MutexDep>>> = OnceLock::new();
@@ -303,11 +318,17 @@ mod tests {
         let c = LazyMutexId::new();
 
         let mut graph = get_dependency_graph();
-        assert!(graph.add_edge(a.value(), b.value(), MutexDep::capture));
-        assert!(graph.add_edge(b.value(), c.value(), MutexDep::capture));
+        assert!(graph
+            .add_edge(a.value(), b.value(), MutexDep::capture)
+            .is_ok());
+        assert!(graph
+            .add_edge(b.value(), c.value(), MutexDep::capture)
+            .is_ok());
 
         // Creating an edge c → a should fail as it introduces a cycle.
-        assert!(!graph.add_edge(c.value(), a.value(), MutexDep::capture));
+        assert!(graph
+            .add_edge(c.value(), a.value(), MutexDep::capture)
+            .is_err());
 
         // Drop graph handle so we can drop vertices without deadlocking
         drop(graph);
@@ -315,7 +336,9 @@ mod tests {
         drop(b);
 
         // If b's destructor correctly ran correctly we can now add an edge from c to a.
-        assert!(get_dependency_graph().add_edge(c.value(), a.value(), MutexDep::capture));
+        assert!(get_dependency_graph()
+            .add_edge(c.value(), a.value(), MutexDep::capture)
+            .is_ok());
     }
 
     /// Test creating a cycle, then panicking.

src/stdsync.rs

@@ -696,7 +696,7 @@ pub mod tracing {
         }
 
         #[test]
-        #[should_panic(expected = "Mutex order graph should not have cycles")]
+        #[should_panic(expected = "Found cycle in mutex dependency graph")]
         fn test_detect_cycle() {
             let a = Mutex::new(());
             let b = Mutex::new(());