diff --git a/signal-hook-registry/src/lib.rs b/signal-hook-registry/src/lib.rs index 1ba8d43..27b9db8 100644 --- a/signal-hook-registry/src/lib.rs +++ b/signal-hook-registry/src/lib.rs @@ -65,9 +65,8 @@ extern crate libc; mod half_lock; +mod vec_map; -use std::collections::hash_map::Entry; -use std::collections::{BTreeMap, HashMap}; use std::io::Error; use std::mem; use std::ptr; @@ -89,6 +88,7 @@ use libc::{SIGFPE, SIGILL, SIGKILL, SIGSEGV, SIGSTOP}; use libc::{SIGFPE, SIGILL, SIGSEGV}; use half_lock::HalfLock; +use vec_map::VecMap; // These constants are not defined in the current version of libc, but it actually // exists in Windows CRT. @@ -140,9 +140,8 @@ type Action = Fn(&siginfo_t) + Send + Sync; #[derive(Clone)] struct Slot { prev: Prev, - // We use BTreeMap here, because we want to run the actions in the order they were inserted. - // This works, because the ActionIds are assigned in an increasing order. - actions: BTreeMap>, + // Actions are stored and executed in the order they were registered. + actions: VecMap>, } impl Slot { @@ -154,7 +153,7 @@ impl Slot { } Ok(Slot { prev: Prev { signal, info: old }, - actions: BTreeMap::new(), + actions: VecMap::new(), }) } @@ -200,14 +199,14 @@ impl Slot { } Ok(Slot { prev: Prev { signal, info: old }, - actions: BTreeMap::new(), + actions: VecMap::new(), }) } } #[derive(Clone)] struct SignalData { - signals: HashMap, + signals: VecMap, next_id: u128, } @@ -324,7 +323,7 @@ impl GlobalData { GLOBAL_INIT.call_once(|| { let data = Box::into_raw(Box::new(GlobalData { data: HalfLock::new(SignalData { - signals: HashMap::new(), + signals: VecMap::new(), next_id: 1, }), race_fallback: HalfLock::new(None), @@ -631,34 +630,32 @@ unsafe fn register_unchecked_impl(signal: c_int, action: Arc) -> Result< let id = ActionId(sigdata.next_id); sigdata.next_id += 1; - match sigdata.signals.entry(signal) { - Entry::Occupied(mut occupied) => { - assert!(occupied.get_mut().actions.insert(id, action).is_none()); - } - Entry::Vacant(place) => { - // While the sigaction/signal exchanges the old one atomically, we are not able to - // atomically store it somewhere a signal handler could read it. That poses a race - // condition where we could lose some signals delivered in between changing it and - // storing it. - // - // Therefore we first store the old one in the fallback storage. The fallback only - // covers the cases where the slot is not yet active and becomes "inert" after that, - // even if not removed (it may get overwritten by some other signal, but for that the - // mutex in globals.data must be unlocked here - and by that time we already stored the - // slot. - // - // And yes, this still leaves a short race condition when some other thread could - // replace the signal handler and we would be calling the outdated one for a short - // time, until we install the slot. - globals - .race_fallback - .write() - .store(Some(Prev::detect(signal)?)); - - let mut slot = Slot::new(signal)?; - slot.actions.insert(id, action); - place.insert(slot); - } + if sigdata.signals.contains(&signal) { + let slot = sigdata.signals.get_mut(&signal).unwrap(); + assert!(slot.actions.insert(id, action).is_none()); + } else { + // While the sigaction/signal exchanges the old one atomically, we are not able to + // atomically store it somewhere a signal handler could read it. That poses a race + // condition where we could lose some signals delivered in between changing it and + // storing it. + // + // Therefore we first store the old one in the fallback storage. The fallback only + // covers the cases where the slot is not yet active and becomes "inert" after that, + // even if not removed (it may get overwritten by some other signal, but for that the + // mutex in globals.data must be unlocked here - and by that time we already stored the + // slot. + // + // And yes, this still leaves a short race condition when some other thread could + // replace the signal handler and we would be calling the outdated one for a short + // time, until we install the slot. + globals + .race_fallback + .write() + .store(Some(Prev::detect(signal)?)); + + let mut slot = Slot::new(signal)?; + slot.actions.insert(id, action); + sigdata.signals.insert(signal, slot); } lock.store(sigdata); diff --git a/signal-hook-registry/src/vec_map.rs b/signal-hook-registry/src/vec_map.rs new file mode 100644 index 0000000..d1e202e --- /dev/null +++ b/signal-hook-registry/src/vec_map.rs @@ -0,0 +1,171 @@ +use std::mem; + +/// A small map backed by an unsorted vector. +/// +/// Maintains key uniqueness at cost of O(n) lookup/insert/remove. Maintains insertion order +/// (`insert` calls that overwrite an existing value don't change order). +#[derive(Clone, Default)] +pub struct VecMap(Vec<(K, V)>); + +impl VecMap { + pub fn new() -> Self { + VecMap(Vec::new()) + } + + pub fn is_empty(&self) -> bool { + self.0.is_empty() + } + + pub fn clear(&mut self) { + self.0.clear(); + } + + fn find(&self, key: &K) -> Option { + for (i, (k, _)) in self.0.iter().enumerate() { + if k == key { + return Some(i); + } + } + None + } + + pub fn contains(&self, key: &K) -> bool { + self.find(key).is_some() + } + + pub fn get(&self, key: &K) -> Option<&V> { + match self.find(key) { + Some(i) => Some(&self.0[i].1), + None => None, + } + } + + pub fn get_mut(&mut self, key: &K) -> Option<&mut V> { + match self.find(key) { + Some(i) => Some(&mut self.0[i].1), + None => None, + } + } + + pub fn insert(&mut self, key: K, value: V) -> Option { + if let Some(old) = self.get_mut(&key) { + return Some(mem::replace(old, value)); + } + self.0.push((key, value)); + None + } + + pub fn remove(&mut self, key: &K) -> Option { + match self.find(key) { + Some(i) => Some(self.0.remove(i).1), + None => None, + } + } + + pub fn values(&self) -> impl Iterator { + self.0.iter().map(|kv| &kv.1) + } +} + +#[cfg(test)] +mod tests { + use super::*; + + #[test] + fn empty() { + let m: VecMap = VecMap::new(); + assert!(m.is_empty()); + assert!(!m.contains(&'a')); + assert!(m.values().next().is_none()); + } + + #[test] + fn insert_update_get() { + let mut m: VecMap = VecMap::new(); + assert!(m.insert('a', 1).is_none()); + assert!(m.insert('b', 2).is_none()); + assert_eq!(m.get(&'a'), Some(&1)); + assert_eq!(m.get(&'b'), Some(&2)); + *m.get_mut(&'a').unwrap() += 10; + assert_eq!(m.get(&'a'), Some(&11)); + assert_eq!(m.get(&'b'), Some(&2)); + } + + #[test] + fn insert_overwrite() { + let mut m = VecMap::new(); + assert_eq!(m.insert('a', 1), None); + assert_eq!(m.insert('b', 2), None); + assert_eq!(m.insert('a', 3), Some(1)); + assert_eq!(m.insert('a', 4), Some(3)); + assert_eq!(m.get(&'a').copied(), Some(4)); + assert_eq!(m.get(&'b').copied(), Some(2)); + assert_eq!(m.insert('b', 5), Some(2)); + assert_eq!(m.get(&'a').copied(), Some(4)); + assert_eq!(m.get(&'b').copied(), Some(5)); + } + + #[test] + fn insert_remove() { + let mut m: VecMap = VecMap::new(); + assert_eq!(m.remove(&'a'), None); + m.insert('a', 1); + m.insert('b', 2); + assert_eq!(m.remove(&'a'), Some(1)); + assert_eq!(m.remove(&'a'), None); + assert_eq!(m.remove(&'b'), Some(2)); + assert!(m.is_empty()); + } + + #[test] + fn insertion_order() { + let mut m: VecMap = VecMap::new(); + let values = |m: &VecMap| -> Vec { m.values().copied().collect() }; + m.insert('b', 2); + m.insert('a', 1); + m.insert('c', 3); + assert_eq!(values(&m), vec![2, 1, 3]); + m.insert('a', 11); + m.remove(&'b'); + assert_eq!(values(&m), vec![11, 3]); + m.insert('b', 2); + assert_eq!(values(&m), vec![11, 3, 2]); + } + + #[test] + fn containment_equivalences() { + let mut m = VecMap::new(); + for i in 0u8..=255 { + if i % 10 < 3 { + m.insert(i, i); + } + } + for i in 0u8..=255 { + if m.contains(&i) { + assert_eq!(m.get(&i).copied(), Some(i)); + assert_eq!(m.get_mut(&i).copied(), Some(i)); + assert_eq!(m.insert(i, i), Some(i)); + assert_eq!(m.remove(&i), Some(i)); + } else { + assert!(m.get(&i).is_none()); + assert!(m.get_mut(&i).is_none()); + assert!(m.remove(&i).is_none()); + assert!(m.insert(i, i).is_none()); + } + } + } + + #[test] + fn clear() { + let mut m = VecMap::new(); + m.clear(); + assert!(m.is_empty()); + m.insert('a', 1); + m.insert('b', 2); + assert!(!m.is_empty()); + m.clear(); + assert!(m.is_empty()); + m.insert('c', 3); + assert!(!m.is_empty()); + } +}