From 5f2e0e99a8851ac0bdd726ba0413e819b414fd18 Mon Sep 17 00:00:00 2001 From: Bert Peters Date: Fri, 19 Mar 2021 21:18:50 +0100 Subject: [PATCH] Use dedicated type for Mutex IDs This should prevent tiny mistakes in handling the value. --- src/graph.rs | 44 +++++++++++++++++++----------- src/lib.rs | 72 ++++++++++++++++++++++++++++++++++++++------------ src/stdsync.rs | 18 ++++--------- 3 files changed, 89 insertions(+), 45 deletions(-) diff --git a/src/graph.rs b/src/graph.rs index 19ba2a4..fe2f9d0 100644 --- a/src/graph.rs +++ b/src/graph.rs @@ -1,21 +1,23 @@ use std::collections::HashMap; use std::collections::HashSet; +use crate::MutexID; + #[derive(Clone, Default, Debug)] pub struct DiGraph { - in_edges: HashMap>, - out_edges: HashMap>, + in_edges: HashMap>, + out_edges: HashMap>, } impl DiGraph { - fn add_node(&mut self, node: usize) -> (&mut Vec, &mut Vec) { + fn add_node(&mut self, node: MutexID) -> (&mut Vec, &mut Vec) { let in_edges = self.in_edges.entry(node).or_default(); let out_edges = self.out_edges.entry(node).or_default(); (in_edges, out_edges) } - pub fn remove_node(&mut self, node: usize) -> bool { + pub(crate) fn remove_node(&mut self, node: MutexID) -> bool { match self.out_edges.remove(&node) { None => false, Some(out_edges) => { @@ -38,7 +40,10 @@ impl DiGraph { } } - pub fn add_edge(&mut self, from: usize, to: usize) -> bool { + /// Add an edge to the graph + /// + /// Nodes, both from and to, are created as needed when creating new edges. + pub(crate) fn add_edge(&mut self, from: MutexID, to: MutexID) -> bool { if from == to { return false; } @@ -65,7 +70,12 @@ impl DiGraph { .any(|node| !self.visit(node, &mut marks, &mut temp)) } - fn visit(&self, node: usize, marks: &mut HashSet, temp: &mut HashSet) -> bool { + fn visit( + &self, + node: MutexID, + marks: &mut HashSet, + temp: &mut HashSet, + ) -> bool { if marks.contains(&node) { return true; } @@ -92,25 +102,29 @@ impl DiGraph { #[cfg(test)] mod tests { - use super::DiGraph; + use super::*; + use crate::MutexID; #[test] fn test_digraph() { + let id: Vec = (0..5).map(|_| MutexID::new()).collect(); let mut graph = DiGraph::default(); - graph.add_edge(1, 2); - graph.add_edge(2, 3); - graph.add_edge(3, 4); - graph.add_edge(5, 2); + // Add some safe edges + graph.add_edge(id[0], id[1]); + graph.add_edge(id[1], id[2]); + graph.add_edge(id[2], id[3]); + graph.add_edge(id[4], id[2]); + // Should not have a cycle yet assert!(!graph.has_cycles()); - graph.add_edge(4, 2); - + // Introduce cycle 3 → 1 → 2 → 3 + graph.add_edge(id[3], id[1]); assert!(graph.has_cycles()); - assert!(graph.remove_node(4)); - + // Removing 3 should remove that cycle + assert!(graph.remove_node(id[3])); assert!(!graph.has_cycles()) } } diff --git a/src/lib.rs b/src/lib.rs index 71d7642..8a579e7 100644 --- a/src/lib.rs +++ b/src/lib.rs @@ -13,6 +13,8 @@ mod graph; pub mod stdsync; /// Counter for Mutex IDs. Atomic avoids the need for locking. +/// +/// Should be part of the `MutexID` impl but static items are not yet a thing. static ID_SEQUENCE: AtomicUsize = AtomicUsize::new(0); thread_local! { @@ -20,17 +22,40 @@ thread_local! { /// /// Assuming that locks are roughly released in the reverse order in which they were acquired, /// a stack should be more efficient to keep track of the current state than a set would be. - static HELD_LOCKS: RefCell> = RefCell::new(Vec::new()); + static HELD_LOCKS: RefCell> = RefCell::new(Vec::new()); } lazy_static! { static ref DEPENDENCY_GRAPH: Mutex = Default::default(); } -fn next_mutex_id() -> usize { - ID_SEQUENCE - .fetch_update(Ordering::SeqCst, Ordering::SeqCst, |id| id.checked_add(1)) - .expect("Mutex ID wraparound happened, results unreliable") +/// Dedicated ID type for Mutexes +/// +/// # Unstable +/// +/// This type is currently private to prevent usage while the exact implementation is figured out, +/// but it will likely be public in the future. +/// +/// One possible alteration is to make this type not `Copy` but `Drop`, and handle deregistering +/// the lock from there. +#[derive(Copy, Clone, Debug, PartialEq, Eq, Hash)] +struct MutexID(usize); + +impl MutexID { + /// Get a new, unique, mutex ID. + /// + /// This ID is guaranteed to be unique within the runtime of the program. + /// + /// # Panics + /// + /// This function may panic when there are no more mutex IDs available. The number of mutex ids + /// is `usize::MAX - 1` which should be plenty for most practical applications. + pub fn new() -> Self { + ID_SEQUENCE + .fetch_update(Ordering::SeqCst, Ordering::SeqCst, |id| id.checked_add(1)) + .map(|id| Self(id)) + .expect("Mutex ID wraparound happened, results unreliable") + } } /// Get a reference to the current dependency graph @@ -41,11 +66,17 @@ fn get_depedency_graph() -> impl DerefMut { } /// Register that a lock is currently held -fn register_lock(lock: usize) { +fn register_lock(lock: MutexID) { HELD_LOCKS.with(|locks| locks.borrow_mut().push(lock)) } -fn drop_lock(id: usize) { +/// Drop a lock held by the current thread. +/// +/// # Panics +/// +/// This function panics if the lock did not appear to be handled by this thread. If that happens, +/// that is an indication of a serious design flaw in this library. +fn drop_lock(id: MutexID) { HELD_LOCKS.with(|locks| { let mut locks = locks.borrow_mut(); @@ -56,7 +87,7 @@ fn drop_lock(id: usize) { } } - panic!("Tried to drop lock for mutex {} but it wasn't held", id) + panic!("Tried to drop lock for mutex {:?} but it wasn't held", id) }); } @@ -64,16 +95,23 @@ fn drop_lock(id: usize) { /// /// If the dependency is new, check for cycles in the dependency graph. If not, there shouldn't be /// any cycles so we don't need to check. -fn register_dependency(lock: usize) { - HELD_LOCKS.with(|locks| { +/// +/// # Panics +/// +/// This function panics if the new dependency would introduce a cycle. +fn register_dependency(lock: MutexID) { + if HELD_LOCKS.with(|locks| { if let Some(&previous) = locks.borrow().last() { let mut graph = get_depedency_graph(); - if graph.add_edge(previous, lock) && graph.has_cycles() { - panic!("Mutex order graph should not have cycles"); - } + graph.add_edge(previous, lock) && graph.has_cycles() + } else { + false } - }) + }) { + // Panic without holding the lock to avoid needlessly poisoning it + panic!("Mutex order graph should not have cycles"); + } } #[cfg(test)] @@ -87,10 +125,10 @@ mod tests { #[test] fn test_next_mutex_id() { - let initial = next_mutex_id(); - let next = next_mutex_id(); + let initial = MutexID::new(); + let next = MutexID::new(); // Can't assert N + 1 because multiple threads running tests - assert!(initial < next); + assert!(initial.0 < next.0); } } diff --git a/src/stdsync.rs b/src/stdsync.rs index 2d3dd5f..1afed3f 100644 --- a/src/stdsync.rs +++ b/src/stdsync.rs @@ -30,15 +30,15 @@ use std::sync::TryLockResult; use crate::drop_lock; use crate::get_depedency_graph; -use crate::next_mutex_id; use crate::register_dependency; use crate::register_lock; +use crate::MutexID; /// Wrapper for `std::sync::Mutex` #[derive(Debug)] pub struct TracingMutex { inner: Mutex, - id: usize, + id: MutexID, } /// Wrapper for `std::sync::MutexGuard` @@ -75,7 +75,7 @@ impl TracingMutex { pub fn new(t: T) -> Self { Self { inner: Mutex::new(t), - id: next_mutex_id(), + id: MutexID::new(), } } @@ -107,10 +107,6 @@ impl TracingMutex { map_trylockresult(result, mapper) } - pub fn get_id(&self) -> usize { - self.id - } - pub fn is_poisoned(&self) -> bool { self.inner.is_poisoned() } @@ -182,7 +178,7 @@ impl<'a, T> Drop for TracingMutexGuard<'a, T> { #[derive(Debug)] pub struct TracingRwLock { inner: RwLock, - id: usize, + id: MutexID, } /// Hybrid wrapper for both `std::sync::RwLockReadGuard` and `std::sync::RwLockWriteGuard`. @@ -203,14 +199,10 @@ impl TracingRwLock { pub fn new(t: T) -> Self { Self { inner: RwLock::new(t), - id: next_mutex_id(), + id: MutexID::new(), } } - pub fn get_id(&self) -> usize { - self.id - } - #[track_caller] pub fn read(&self) -> LockResult> { register_dependency(self.id);