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/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/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 2ad2765..dbd0f8f 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,23 +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>, - /// Next topological sort order - next_ord: Order, + 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 @@ -43,7 +45,7 @@ where ord: Cell, } -impl DiGraph +impl DiGraph where V: Eq + Hash + Copy, { @@ -54,12 +56,18 @@ 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) { - let next_ord = &mut self.next_ord; + 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(); 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,9 +85,12 @@ where Some(Node { out_edges, in_edges, - .. + ord, }) => { - out_edges.into_iter().for_each(|m| { + // Return ordering to the pool of unused ones + self.unused_order.push(ord.get()); + + out_edges.into_keys().for_each(|m| { self.nodes.get_mut(&m).unwrap().in_edges.remove(&n); }); @@ -96,18 +107,29 @@ 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 { + /// + /// # 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); - 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 Ok(()); + } + Entry::Vacant(entry) => entry.insert(e()), + }; let (in_edges, _, lb) = self.add_node(y); @@ -119,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. @@ -129,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 @@ -141,44 +163,49 @@ where self.reorder(delta_f, delta_b); } - true + Ok(()) } /// 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>, - ) -> bool { + delta_f: &mut Vec<&'a Node>, + ) -> Result<(), Vec> + where + E: Clone, + { delta_f.push(n); - n.out_edges.iter().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 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); @@ -192,7 +219,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); @@ -213,12 +240,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; @@ -226,12 +266,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).is_err()); } #[test] @@ -239,16 +281,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).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)); + 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)); + assert!(graph.add_edge(4, 0, nop).is_ok()); } /// Fuzz the DiGraph implementation by adding a bunch of valid edges. @@ -256,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 @@ -277,7 +319,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).is_ok()); } } } diff --git a/src/lib.rs b/src/lib.rs index 245191c..9c958eb 100644 --- a/src/lib.rs +++ b/src/lib.rs @@ -64,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; @@ -74,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! { @@ -137,19 +140,18 @@ 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()) + graph.add_edge(previous, self.value(), Dep::capture).err() } else { - false + None } }); - if creates_cycle { - // Panic without holding the lock to avoid needlessly poisoning it - panic!("Mutex order graph should not have cycles"); + if let Some(cycle) = opt_cycle { + panic!("{}", Dep::panic_message(&cycle)) } HELD_LOCKS.with(|locks| locks.borrow_mut().push(self.value())); @@ -260,8 +262,8 @@ impl<'a> Drop for BorrowedMutex<'a> { } /// 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 +294,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(), 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())); + assert!(graph.add_edge(c.value(), a.value(), Dep::capture).is_err()); // Drop graph handle so we can drop vertices without deadlocking drop(graph); @@ -304,7 +306,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())); + assert!(get_dependency_graph() + .add_edge(c.value(), a.value(), Dep::capture) + .is_ok()); } /// Test creating a cycle, then panicking. 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() + } +} 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(());