From 99ef9bfbc108b345301c885219b4dd1d4822e261 Mon Sep 17 00:00:00 2001 From: DaniPopes <57450786+DaniPopes@users.noreply.github.com> Date: Sat, 30 May 2026 06:27:47 +0200 Subject: [PATCH 1/4] fix: drop entries before clear_slow clear_slow zeroed bucket storage without first dropping initialized entries. For K or V types that own heap data, this leaked resources and prevented Bucket::drop from observing the old ALIVE_BIT later. Drop the bucket slice when entries need drop before reinitializing the storage with zeroed buckets, and add a regression test that tracks drops across clear_slow and cache drop. --- src/lib.rs | 44 ++++++++++++++++++++++++++++++++++++++++++-- 1 file changed, 42 insertions(+), 2 deletions(-) diff --git a/src/lib.rs b/src/lib.rs index 57efe25..afc9eae 100644 --- a/src/lib.rs +++ b/src/lib.rs @@ -274,9 +274,12 @@ where pub fn clear_slow(&self) { // SAFETY: We zero the entire bucket array. This is safe because: // - Bucket is repr(C) and zeroed memory is a valid empty bucket state. - // - We don't need to drop existing entries since we're zeroing the ALIVE_BIT. - // - Concurrent readers will see either the old state or zeros (empty). + // - Existing entries are dropped before their ALIVE_BIT is zeroed. + // - Callers ensure no other threads are accessing the cache. unsafe { + if Self::NEEDS_DROP { + ptr::drop_in_place(self.entries.cast_mut()); + } ptr::write_bytes( self.entries.cast_mut().cast::>(), 0, @@ -1265,6 +1268,43 @@ mod tests { assert_eq!(cache.get("key"), Some("value2".to_string())); } + #[test] + fn clear_slow_drops_entries() { + use std::sync::atomic::{AtomicUsize, Ordering}; + + static DROP_COUNT: AtomicUsize = AtomicUsize::new(0); + + #[derive(Clone, Hash, Eq, PartialEq)] + struct DropKey(u64); + impl Drop for DropKey { + fn drop(&mut self) { + DROP_COUNT.fetch_add(1, Ordering::SeqCst); + } + } + + #[derive(Clone)] + struct DropValue(#[allow(dead_code)] u64); + impl Drop for DropValue { + fn drop(&mut self) { + DROP_COUNT.fetch_add(1, Ordering::SeqCst); + } + } + + DROP_COUNT.store(0, Ordering::SeqCst); + { + let cache: Cache = new_cache(64); + cache.insert(DropKey(1), DropValue(100)); + assert_eq!(DROP_COUNT.load(Ordering::SeqCst), 0); + + cache.clear_slow(); + assert_eq!(DROP_COUNT.load(Ordering::SeqCst), 2); + + cache.insert(DropKey(1), DropValue(200)); + assert_eq!(DROP_COUNT.load(Ordering::SeqCst), 2); + } + assert_eq!(DROP_COUNT.load(Ordering::SeqCst), 4); + } + #[test] fn epoch_remove() { let cache: EpochCache = EpochCache::new(4096, Default::default()); From 861f2351708e54dba6c1f0c88833c7947e2434be Mon Sep 17 00:00:00 2001 From: DaniPopes <57450786+DaniPopes@users.noreply.github.com> Date: Sat, 30 May 2026 06:28:51 +0200 Subject: [PATCH 2/4] refactor: clear dropped buckets in one pass --- src/lib.rs | 14 ++++++++------ 1 file changed, 8 insertions(+), 6 deletions(-) diff --git a/src/lib.rs b/src/lib.rs index afc9eae..f17ef2f 100644 --- a/src/lib.rs +++ b/src/lib.rs @@ -277,14 +277,16 @@ where // - Existing entries are dropped before their ALIVE_BIT is zeroed. // - Callers ensure no other threads are accessing the cache. unsafe { + let entries = self.entries.cast_mut().cast::>(); if Self::NEEDS_DROP { - ptr::drop_in_place(self.entries.cast_mut()); + for i in 0..self.entries.len() { + let entry = entries.add(i); + ptr::drop_in_place(entry); + ptr::write_bytes(entry, 0, 1); + } + } else { + ptr::write_bytes(entries, 0, self.entries.len()); } - ptr::write_bytes( - self.entries.cast_mut().cast::>(), - 0, - self.entries.len(), - ) }; } From c5630e385deb58492227087f18945d0bffa0d68e Mon Sep 17 00:00:00 2001 From: DaniPopes <57450786+DaniPopes@users.noreply.github.com> Date: Sat, 30 May 2026 06:57:30 +0200 Subject: [PATCH 3/4] perf: clear bucket tags directly --- src/lib.rs | 25 ++++++++++++++----------- 1 file changed, 14 insertions(+), 11 deletions(-) diff --git a/src/lib.rs b/src/lib.rs index f17ef2f..627201e 100644 --- a/src/lib.rs +++ b/src/lib.rs @@ -262,7 +262,7 @@ where } } - /// Clears the cache by zeroing all bucket memory. + /// Clears the cache by invalidating all buckets. /// /// This is O(N) where N is the number of buckets. Prefer [`clear`](Self::clear) when /// [`CacheConfig::EPOCHS`] is `true`. @@ -271,21 +271,24 @@ where /// /// This method is safe but may race with concurrent operations. Callers should ensure /// no other threads are accessing the cache during this operation. + #[inline(never)] pub fn clear_slow(&self) { - // SAFETY: We zero the entire bucket array. This is safe because: - // - Bucket is repr(C) and zeroed memory is a valid empty bucket state. - // - Existing entries are dropped before their ALIVE_BIT is zeroed. - // - Callers ensure no other threads are accessing the cache. + // SAFETY: Callers ensure no other threads are accessing the cache. Existing entries are + // dropped after clearing their tags so the bucket won't be dropped again if dropping + // panics. unsafe { - let entries = self.entries.cast_mut().cast::>(); if Self::NEEDS_DROP { - for i in 0..self.entries.len() { - let entry = entries.add(i); - ptr::drop_in_place(entry); - ptr::write_bytes(entry, 0, 1); + for entry in &*self.entries { + let tag = entry.tag.load(Ordering::Relaxed); + entry.tag.store(0, Ordering::Relaxed); + if tag & ALIVE_BIT != 0 { + (*entry.data.get()).assume_init_drop(); + } } } else { - ptr::write_bytes(entries, 0, self.entries.len()); + for entry in &*self.entries { + entry.tag.store(0, Ordering::Relaxed); + } } }; } From a9fe0d97c14d500dd88187abbabab2c904ead160 Mon Sep 17 00:00:00 2001 From: DaniPopes <57450786+DaniPopes@users.noreply.github.com> Date: Sat, 30 May 2026 07:07:07 +0200 Subject: [PATCH 4/4] test: simplify drop counting tests --- src/lib.rs | 194 +++++++++++++++++++++++++++++------------------------ 1 file changed, 106 insertions(+), 88 deletions(-) diff --git a/src/lib.rs b/src/lib.rs index 627201e..410ea17 100644 --- a/src/lib.rs +++ b/src/lib.rs @@ -273,21 +273,14 @@ where /// no other threads are accessing the cache during this operation. #[inline(never)] pub fn clear_slow(&self) { - // SAFETY: Callers ensure no other threads are accessing the cache. Existing entries are - // dropped after clearing their tags so the bucket won't be dropped again if dropping - // panics. + // SAFETY: Callers ensure no other threads are accessing the cache. unsafe { - if Self::NEEDS_DROP { - for entry in &*self.entries { - let tag = entry.tag.load(Ordering::Relaxed); - entry.tag.store(0, Ordering::Relaxed); - if tag & ALIVE_BIT != 0 { - (*entry.data.get()).assume_init_drop(); - } - } - } else { - for entry in &*self.entries { - entry.tag.store(0, Ordering::Relaxed); + for entry in &*self.entries { + let is_alive = Self::NEEDS_DROP && entry.is_alive(); + // Store before dropping to avoid double-dropping if dropping panics. + entry.tag.store(0, Ordering::Relaxed); + if is_alive { + (*entry.data.get()).assume_init_drop(); } } }; @@ -672,7 +665,7 @@ macro_rules! static_cache { #[cfg(test)] mod tests { use super::*; - use std::thread; + use std::{cell::Cell, rc::Rc, thread}; const fn iters(n: usize) -> usize { if cfg!(miri) { n / 10 } else { n } @@ -697,6 +690,63 @@ mod tests { Cache::new(size, Default::default()) } + type Drops = Rc>; + + fn drops() -> Drops { + Rc::new(Cell::new(0)) + } + + #[derive(Clone)] + struct DropKey { + id: u64, + drops: Drops, + } + + impl DropKey { + fn new(id: u64, drops: &Drops) -> Self { + Self { id, drops: drops.clone() } + } + } + + impl Hash for DropKey { + fn hash(&self, state: &mut H) { + self.id.hash(state); + } + } + + impl PartialEq for DropKey { + fn eq(&self, other: &Self) -> bool { + self.id == other.id + } + } + + impl Eq for DropKey {} + + impl Drop for DropKey { + fn drop(&mut self) { + self.drops.set(self.drops.get() + 1); + } + } + + #[derive(Clone)] + struct DropValue { + #[allow(dead_code)] + value: u64, + drops: Drops, + } + + impl DropValue { + fn new(value: u64, drops: &Drops) -> Self { + Self { value, drops: drops.clone() } + } + } + + impl Drop for DropValue { + fn drop(&mut self) { + self.drops.set(self.drops.get() + 1); + } + } + #[test] fn basic_get_or_insert() { let cache = new_cache(1024); @@ -1077,74 +1127,34 @@ mod tests { #[test] fn drop_on_cache_drop() { - use std::sync::atomic::{AtomicUsize, Ordering}; - - static DROP_COUNT: AtomicUsize = AtomicUsize::new(0); - - #[derive(Clone, Hash, Eq, PartialEq)] - struct DropKey(u64); - impl Drop for DropKey { - fn drop(&mut self) { - DROP_COUNT.fetch_add(1, Ordering::SeqCst); - } - } - - #[derive(Clone)] - struct DropValue(#[allow(dead_code)] u64); - impl Drop for DropValue { - fn drop(&mut self) { - DROP_COUNT.fetch_add(1, Ordering::SeqCst); - } - } - - DROP_COUNT.store(0, Ordering::SeqCst); + let drops = drops(); { let cache: super::Cache = super::Cache::new(64, Default::default()); - cache.insert(DropKey(1), DropValue(100)); - cache.insert(DropKey(2), DropValue(200)); - cache.insert(DropKey(3), DropValue(300)); - assert_eq!(DROP_COUNT.load(Ordering::SeqCst), 0); + cache.insert(DropKey::new(1, &drops), DropValue::new(100, &drops)); + cache.insert(DropKey::new(2, &drops), DropValue::new(200, &drops)); + cache.insert(DropKey::new(3, &drops), DropValue::new(300, &drops)); + assert_eq!(drops.get(), 0); } // 3 keys + 3 values = 6 drops - assert_eq!(DROP_COUNT.load(Ordering::SeqCst), 6); + assert_eq!(drops.get(), 6); } #[test] fn drop_on_eviction() { - use std::sync::atomic::{AtomicUsize, Ordering}; - - static DROP_COUNT: AtomicUsize = AtomicUsize::new(0); - - #[derive(Clone, Hash, Eq, PartialEq)] - struct DropKey(u64); - impl Drop for DropKey { - fn drop(&mut self) { - DROP_COUNT.fetch_add(1, Ordering::SeqCst); - } - } - - #[derive(Clone)] - struct DropValue(#[allow(dead_code)] u64); - impl Drop for DropValue { - fn drop(&mut self) { - DROP_COUNT.fetch_add(1, Ordering::SeqCst); - } - } - - DROP_COUNT.store(0, Ordering::SeqCst); + let drops = drops(); { let cache: super::Cache = super::Cache::new(64, Default::default()); - cache.insert(DropKey(1), DropValue(100)); - assert_eq!(DROP_COUNT.load(Ordering::SeqCst), 0); + cache.insert(DropKey::new(1, &drops), DropValue::new(100, &drops)); + assert_eq!(drops.get(), 0); // Insert same key again - should evict old entry - cache.insert(DropKey(1), DropValue(200)); + cache.insert(DropKey::new(1, &drops), DropValue::new(200, &drops)); // Old key + old value dropped = 2 - assert_eq!(DROP_COUNT.load(Ordering::SeqCst), 2); + assert_eq!(drops.get(), 2); } // Cache dropped: new key + new value = 2 more - assert_eq!(DROP_COUNT.load(Ordering::SeqCst), 4); + assert_eq!(drops.get(), 4); } #[test] @@ -1275,39 +1285,47 @@ mod tests { #[test] fn clear_slow_drops_entries() { - use std::sync::atomic::{AtomicUsize, Ordering}; + let drops = drops(); + { + let cache: Cache = new_cache(64); + cache.insert(DropKey::new(1, &drops), DropValue::new(100, &drops)); + assert_eq!(drops.get(), 0); - static DROP_COUNT: AtomicUsize = AtomicUsize::new(0); + cache.clear_slow(); + assert_eq!(drops.get(), 2); - #[derive(Clone, Hash, Eq, PartialEq)] - struct DropKey(u64); - impl Drop for DropKey { - fn drop(&mut self) { - DROP_COUNT.fetch_add(1, Ordering::SeqCst); - } + cache.insert(DropKey::new(1, &drops), DropValue::new(200, &drops)); + assert_eq!(drops.get(), 2); } + assert_eq!(drops.get(), 4); + } + + #[test] + fn clear_slow_panic_does_not_double_drop_entry() { + use std::panic::{AssertUnwindSafe, catch_unwind}; #[derive(Clone)] - struct DropValue(#[allow(dead_code)] u64); - impl Drop for DropValue { + struct PanicOnFirstDrop(Drops); + impl Drop for PanicOnFirstDrop { fn drop(&mut self) { - DROP_COUNT.fetch_add(1, Ordering::SeqCst); + let prev = self.0.get(); + self.0.set(prev + 1); + if prev == 0 { + panic!("intentional panic from drop"); + } } } - DROP_COUNT.store(0, Ordering::SeqCst); + let drops = drops(); { - let cache: Cache = new_cache(64); - cache.insert(DropKey(1), DropValue(100)); - assert_eq!(DROP_COUNT.load(Ordering::SeqCst), 0); - - cache.clear_slow(); - assert_eq!(DROP_COUNT.load(Ordering::SeqCst), 2); + let cache: Cache = new_cache(64); + cache.insert(1, PanicOnFirstDrop(drops.clone())); - cache.insert(DropKey(1), DropValue(200)); - assert_eq!(DROP_COUNT.load(Ordering::SeqCst), 2); + let result = catch_unwind(AssertUnwindSafe(|| cache.clear_slow())); + assert!(result.is_err()); + assert_eq!(drops.get(), 1); } - assert_eq!(DROP_COUNT.load(Ordering::SeqCst), 4); + assert_eq!(drops.get(), 1); } #[test]