From 068303d81d9de8a4b3e16aac06a6ac0a2fc30405 Mon Sep 17 00:00:00 2001 From: Bert Peters Date: Sun, 27 Aug 2023 18:06:11 +0200 Subject: [PATCH] Show cycle backtraces when they happen --- CHANGELOG.md | 1 + examples/mutex_cycle.rs | 26 ++++++++++++++++++ src/graph.rs | 59 +++++++++++++++++++++++++---------------- src/lib.rs | 41 +++++++++++++++++++++------- src/stdsync.rs | 2 +- 5 files changed, 96 insertions(+), 33 deletions(-) create mode 100644 examples/mutex_cycle.rs diff --git a/CHANGELOG.md b/CHANGELOG.md index d88cbc6..e407500 100644 --- a/CHANGELOG.md +++ b/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 diff --git a/examples/mutex_cycle.rs b/examples/mutex_cycle.rs new file mode 100644 index 0000000..94bf075 --- /dev/null +++ b/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 +} diff --git a/src/graph.rs b/src/graph.rs index 284038a..dbd0f8f 100644 --- a/src/graph.rs +++ b/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> + 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, delta_f: &mut Vec<&'a Node>, - ) -> bool { + ) -> Result<(), Vec> + 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()); } } } diff --git a/src/lib.rs b/src/lib.rs index 6d4c29e..59cbd60 100644 --- a/src/lib.rs +++ b/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> { static DEPENDENCY_GRAPH: OnceLock>> = 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. diff --git a/src/stdsync.rs b/src/stdsync.rs index b9b237d..c3ba067 100644 --- a/src/stdsync.rs +++ b/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(());