From 909e934572b0bd4b956ba84b090c4f5f7a6c76dd Mon Sep 17 00:00:00 2001 From: Bert Peters Date: Sun, 27 Aug 2023 15:48:57 +0200 Subject: [PATCH 1/4] Reuse dependency orderings in graph This avoids a potential panic when adding new nodes to the graph, as there is no feasible way to overflow IDs any more. --- src/graph.rs | 22 ++++++++++++++++------ 1 file changed, 16 insertions(+), 6 deletions(-) diff --git a/src/graph.rs b/src/graph.rs index 2ad2765..768c3f8 100644 --- a/src/graph.rs +++ b/src/graph.rs @@ -25,8 +25,9 @@ where V: Eq + Hash + Copy, { nodes: HashMap>, - /// Next topological sort order - next_ord: Order, + // Instead of reordering the orders in the graph whenever a node is deleted, we maintain a list + // of unused ids that can be handed out later again. + unused_order: Vec, } #[derive(Debug)] @@ -55,11 +56,17 @@ where /// /// New nodes are appended to the end of the topological order when added. fn add_node(&mut self, n: V) -> (&mut HashSet, &mut HashSet, Order) { - let next_ord = &mut self.next_ord; + // need to compute next id before the call to entry() to avoid duplicate borrow of nodes + let fallback_id = self.nodes.len(); let node = self.nodes.entry(n).or_insert_with(|| { - let order = *next_ord; - *next_ord = next_ord.checked_add(1).expect("Topological order overflow"); + let order = if let Some(id) = self.unused_order.pop() { + // Reuse discarded ordering entry + id + } else { + // Allocate new order id + fallback_id + }; Node { ord: Cell::new(order), @@ -77,8 +84,11 @@ where Some(Node { out_edges, in_edges, - .. + ord, }) => { + // Return ordering to the pool of unused ones + self.unused_order.push(ord.get()); + out_edges.into_iter().for_each(|m| { self.nodes.get_mut(&m).unwrap().in_edges.remove(&n); }); From 6be3e05cab485ad22f10acd9185d57936c84eb4a Mon Sep 17 00:00:00 2001 From: Bert Peters Date: Sun, 27 Aug 2023 16:44:02 +0200 Subject: [PATCH 2/4] Capture backtraces of allocations for debugging Largely based on https://github.com/bertptrs/tracing-mutex/pull/28 with only minor modifications. --- src/graph.rs | 75 ++++++++++++++++++++++++++++++++-------------------- src/lib.rs | 25 +++++++++++++----- 2 files changed, 65 insertions(+), 35 deletions(-) diff --git a/src/graph.rs b/src/graph.rs index 768c3f8..284038a 100644 --- a/src/graph.rs +++ b/src/graph.rs @@ -1,4 +1,5 @@ use std::cell::Cell; +use std::collections::hash_map::Entry; use std::collections::HashMap; use std::collections::HashSet; use std::hash::Hash; @@ -19,24 +20,24 @@ type Order = usize; /// visibly changed. /// /// [paper]: https://whileydave.com/publications/pk07_jea/ -#[derive(Default, Debug)] -pub struct DiGraph +#[derive(Debug)] +pub struct DiGraph where V: Eq + Hash + Copy, { - nodes: HashMap>, + nodes: HashMap>, // Instead of reordering the orders in the graph whenever a node is deleted, we maintain a list // of unused ids that can be handed out later again. unused_order: Vec, } #[derive(Debug)] -struct Node +struct Node where V: Eq + Hash + Clone, { in_edges: HashSet, - out_edges: HashSet, + out_edges: HashMap, // The "Ord" field is a Cell to ensure we can update it in an immutable context. // `std::collections::HashMap` doesn't let you have multiple mutable references to elements, but // this way we can use immutable references and still update `ord`. This saves quite a few @@ -44,7 +45,7 @@ where ord: Cell, } -impl DiGraph +impl DiGraph where V: Eq + Hash + Copy, { @@ -55,7 +56,7 @@ where /// the node in the topological order. /// /// New nodes are appended to the end of the topological order when added. - fn add_node(&mut self, n: V) -> (&mut HashSet, &mut HashSet, Order) { + fn add_node(&mut self, n: V) -> (&mut HashSet, &mut HashMap, Order) { // need to compute next id before the call to entry() to avoid duplicate borrow of nodes let fallback_id = self.nodes.len(); @@ -89,7 +90,7 @@ where // Return ordering to the pool of unused ones self.unused_order.push(ord.get()); - out_edges.into_iter().for_each(|m| { + out_edges.into_keys().for_each(|m| { self.nodes.get_mut(&m).unwrap().in_edges.remove(&n); }); @@ -106,7 +107,7 @@ 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) -> bool { + pub(crate) fn add_edge(&mut self, x: V, y: V, e: impl FnOnce() -> E) -> bool { if x == y { // self-edges are always considered cycles return false; @@ -114,10 +115,13 @@ where let (_, out_edges, ub) = self.add_node(x); - if !out_edges.insert(y) { - // Edge already exists, nothing to be done - return true; - } + match out_edges.entry(y) { + Entry::Occupied(_) => { + // Edge already exists, nothing to be done + return true; + } + Entry::Vacant(entry) => entry.insert(e()), + }; let (in_edges, _, lb) = self.add_node(y); @@ -157,14 +161,14 @@ where /// Forwards depth-first-search fn dfs_f<'a>( &'a self, - n: &'a Node, + n: &'a Node, ub: Order, visited: &mut HashSet, - delta_f: &mut Vec<&'a Node>, + delta_f: &mut Vec<&'a Node>, ) -> bool { delta_f.push(n); - n.out_edges.iter().all(|w| { + n.out_edges.keys().all(|w| { let node = &self.nodes[w]; let ord = node.ord.get(); @@ -185,10 +189,10 @@ where /// Backwards depth-first-search fn dfs_b<'a>( &'a self, - n: &'a Node, + n: &'a Node, lb: Order, visited: &mut HashSet, - delta_b: &mut Vec<&'a Node>, + delta_b: &mut Vec<&'a Node>, ) { delta_b.push(n); @@ -202,7 +206,7 @@ where } } - fn reorder(&self, mut delta_f: Vec<&Node>, mut delta_b: Vec<&Node>) { + fn reorder(&self, mut delta_f: Vec<&Node>, mut delta_b: Vec<&Node>) { self.sort(&mut delta_f); self.sort(&mut delta_b); @@ -223,12 +227,25 @@ where } } - fn sort(&self, ids: &mut [&Node]) { + fn sort(&self, ids: &mut [&Node]) { // Can use unstable sort because mutex ids should not be equal ids.sort_unstable_by_key(|v| &v.ord); } } +// Manual `Default` impl as derive causes unnecessarily strong bounds. +impl Default for DiGraph +where + V: Eq + Hash + Copy, +{ + fn default() -> Self { + Self { + nodes: Default::default(), + unused_order: Default::default(), + } + } +} + #[cfg(test)] mod tests { use rand::seq::SliceRandom; @@ -236,12 +253,14 @@ mod tests { use super::*; + fn nop() {} + #[test] fn test_no_self_cycle() { // Regression test for https://github.com/bertptrs/tracing-mutex/issues/7 let mut graph = DiGraph::default(); - assert!(!graph.add_edge(1, 1)); + assert!(!graph.add_edge(1, 1, nop)); } #[test] @@ -249,16 +268,16 @@ mod tests { let mut graph = DiGraph::default(); // Add some safe edges - assert!(graph.add_edge(0, 1)); - assert!(graph.add_edge(1, 2)); - assert!(graph.add_edge(2, 3)); - assert!(graph.add_edge(4, 2)); + 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)); // Try to add an edge that introduces a cycle - assert!(!graph.add_edge(3, 1)); + assert!(!graph.add_edge(3, 1, nop)); // Add an edge that should reorder 0 to be after 4 - assert!(graph.add_edge(4, 0)); + assert!(graph.add_edge(4, 0, nop)); } /// Fuzz the DiGraph implementation by adding a bunch of valid edges. @@ -287,7 +306,7 @@ mod tests { let mut graph = DiGraph::default(); for (x, y) in edges { - assert!(graph.add_edge(x, y)); + assert!(graph.add_edge(x, y, nop)); } } } diff --git a/src/lib.rs b/src/lib.rs index 245191c..6d4c29e 100644 --- a/src/lib.rs +++ b/src/lib.rs @@ -46,6 +46,7 @@ //! //! [paper]: https://whileydave.com/publications/pk07_jea/ #![cfg_attr(docsrs, feature(doc_cfg))] +use std::backtrace::Backtrace; use std::cell::RefCell; use std::fmt; use std::marker::PhantomData; @@ -53,6 +54,7 @@ use std::ops::Deref; use std::ops::DerefMut; use std::sync::atomic::AtomicUsize; use std::sync::atomic::Ordering; +use std::sync::Arc; use std::sync::Mutex; use std::sync::MutexGuard; use std::sync::OnceLock; @@ -141,7 +143,7 @@ impl MutexId { if let Some(&previous) = locks.borrow().last() { let mut graph = get_dependency_graph(); - !graph.add_edge(previous, self.value()) + !graph.add_edge(previous, self.value(), MutexDep::capture) } else { false } @@ -259,9 +261,18 @@ impl<'a> Drop for BorrowedMutex<'a> { } } +#[derive(Clone)] +struct MutexDep(Arc); + +impl MutexDep { + pub fn capture() -> Self { + Self(Arc::new(Backtrace::capture())) + } +} + /// Get a reference to the current dependency graph -fn get_dependency_graph() -> impl DerefMut> { - static DEPENDENCY_GRAPH: OnceLock>> = OnceLock::new(); +fn get_dependency_graph() -> impl DerefMut> { + static DEPENDENCY_GRAPH: OnceLock>> = OnceLock::new(); DEPENDENCY_GRAPH .get_or_init(Default::default) @@ -292,11 +303,11 @@ mod tests { let c = LazyMutexId::new(); let mut graph = get_dependency_graph(); - assert!(graph.add_edge(a.value(), b.value())); - assert!(graph.add_edge(b.value(), c.value())); + assert!(graph.add_edge(a.value(), b.value(), MutexDep::capture)); + assert!(graph.add_edge(b.value(), c.value(), MutexDep::capture)); // Creating an edge c → a should fail as it introduces a cycle. - assert!(!graph.add_edge(c.value(), a.value())); + assert!(!graph.add_edge(c.value(), a.value(), MutexDep::capture)); // Drop graph handle so we can drop vertices without deadlocking drop(graph); @@ -304,7 +315,7 @@ 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())); + assert!(get_dependency_graph().add_edge(c.value(), a.value(), MutexDep::capture)); } /// Test creating a cycle, then panicking. From 068303d81d9de8a4b3e16aac06a6ac0a2fc30405 Mon Sep 17 00:00:00 2001 From: Bert Peters Date: Sun, 27 Aug 2023 18:06:11 +0200 Subject: [PATCH 3/4] 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(()); From a8e8af635131faec6ce125064326e5abcc73f6bc Mon Sep 17 00:00:00 2001 From: Bert Peters Date: Sat, 9 Sep 2023 11:10:08 +0200 Subject: [PATCH 4/4] Make dependency tracking a compile time setting --- Cargo.toml | 2 ++ src/lib.rs | 52 +++++++++------------------------------ src/reporting.rs | 64 ++++++++++++++++++++++++++++++++++++++++++++++++ 3 files changed, 77 insertions(+), 41 deletions(-) create mode 100644 src/reporting.rs diff --git a/Cargo.toml b/Cargo.toml index c86b5c5..74db0f7 100644 --- a/Cargo.toml +++ b/Cargo.toml @@ -31,6 +31,8 @@ name = "mutex" harness = false [features] +default = ["backtraces"] +backtraces = [] # Feature names do not match crate names pending namespaced features. lockapi = ["lock_api"] parkinglot = ["parking_lot", "lockapi"] diff --git a/src/lib.rs b/src/lib.rs index 59cbd60..9c958eb 100644 --- a/src/lib.rs +++ b/src/lib.rs @@ -46,7 +46,6 @@ //! //! [paper]: https://whileydave.com/publications/pk07_jea/ #![cfg_attr(docsrs, feature(doc_cfg))] -use std::backtrace::Backtrace; use std::cell::RefCell; use std::fmt; use std::marker::PhantomData; @@ -54,7 +53,6 @@ use std::ops::Deref; use std::ops::DerefMut; use std::sync::atomic::AtomicUsize; use std::sync::atomic::Ordering; -use std::sync::Arc; use std::sync::Mutex; use std::sync::MutexGuard; use std::sync::OnceLock; @@ -66,6 +64,8 @@ pub use lock_api; #[cfg(feature = "parkinglot")] #[cfg_attr(docsrs, doc(cfg(feature = "parkinglot")))] pub use parking_lot; +use reporting::Dep; +use reporting::Reportable; use crate::graph::DiGraph; @@ -76,6 +76,7 @@ pub mod lockapi; #[cfg(feature = "parkinglot")] #[cfg_attr(docsrs, doc(cfg(feature = "parkinglot")))] pub mod parkinglot; +mod reporting; pub mod stdsync; thread_local! { @@ -143,24 +144,14 @@ impl MutexId { if let Some(&previous) = locks.borrow().last() { let mut graph = get_dependency_graph(); - graph - .add_edge(previous, self.value(), MutexDep::capture) - .err() + graph.add_edge(previous, self.value(), Dep::capture).err() } else { None } }); if let Some(cycle) = opt_cycle { - // Panic without holding the lock to avoid needlessly poisoning it - 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}"); + panic!("{}", Dep::panic_message(&cycle)) } HELD_LOCKS.with(|locks| locks.borrow_mut().push(self.value())); @@ -270,24 +261,9 @@ impl<'a> Drop for BorrowedMutex<'a> { } } -#[derive(Clone)] -struct MutexDep(Arc); - -impl MutexDep { - pub fn capture() -> Self { - Self(Arc::new(Backtrace::capture())) - } -} - -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(); +fn get_dependency_graph() -> impl DerefMut> { + static DEPENDENCY_GRAPH: OnceLock>> = OnceLock::new(); DEPENDENCY_GRAPH .get_or_init(Default::default) @@ -318,17 +294,11 @@ mod tests { let c = LazyMutexId::new(); let mut graph = get_dependency_graph(); - assert!(graph - .add_edge(a.value(), b.value(), MutexDep::capture) - .is_ok()); - assert!(graph - .add_edge(b.value(), c.value(), MutexDep::capture) - .is_ok()); + assert!(graph.add_edge(a.value(), b.value(), Dep::capture).is_ok()); + assert!(graph.add_edge(b.value(), c.value(), Dep::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) - .is_err()); + assert!(graph.add_edge(c.value(), a.value(), Dep::capture).is_err()); // Drop graph handle so we can drop vertices without deadlocking drop(graph); @@ -337,7 +307,7 @@ mod tests { // 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) + .add_edge(c.value(), a.value(), Dep::capture) .is_ok()); } diff --git a/src/reporting.rs b/src/reporting.rs new file mode 100644 index 0000000..4149690 --- /dev/null +++ b/src/reporting.rs @@ -0,0 +1,64 @@ +//! Cycle reporting primitives +//! +//! This module exposes [`Dep`], which resolves to either something that tracks dependencies or to +//! something that doesn't. It should only be assumed to implement the [`Reportable`] trait. +use std::backtrace::Backtrace; +use std::borrow::Cow; +use std::fmt::Write; +use std::sync::Arc; + +#[cfg(feature = "backtraces")] +pub type Dep = MutexDep>; +#[cfg(not(feature = "backtraces"))] +pub type Dep = MutexDep<()>; + +// Base message to be reported when cycle is detected +const BASE_MESSAGE: &str = "Found cycle in mutex dependency graph:"; + +pub trait Reportable: Clone { + /// Capture the current state + fn capture() -> Self; + + /// Format a trace of state for human readable consumption. + fn panic_message(trace: &[Self]) -> Cow<'static, str>; +} + +#[derive(Clone)] +pub struct MutexDep(T); + +/// Use a unit as tracing data: no tracing. +/// +/// This should have no runtime overhead for capturing traces and should therefore be cheap enough +/// for most purposes. +impl Reportable for MutexDep<()> { + fn capture() -> Self { + Self(()) + } + + fn panic_message(_trace: &[Self]) -> Cow<'static, str> { + Cow::Borrowed(BASE_MESSAGE) + } +} + +/// Use a full backtrace as tracing data +/// +/// Capture the entire backtrace which may be expensive. This implementation does not force capture +/// in the event that backtraces are disabled at runtime, so the exact overhead can still be +/// controlled a little. +/// +/// N.B. the [`Backtrace`] needs to be wrapped in an Arc as backtraces are not [`Clone`]. +impl Reportable for MutexDep> { + fn capture() -> Self { + Self(Arc::new(Backtrace::capture())) + } + + fn panic_message(trace: &[Self]) -> Cow<'static, str> { + let mut message = format!("{BASE_MESSAGE}\n"); + + for entry in trace { + let _ = writeln!(message, "{}", entry.0); + } + + message.into() + } +}