From 8b5a0047c899e504ced268ac08e6909f02aa349c Mon Sep 17 00:00:00 2001 From: Dmitry Agafonov <42949186+Malkiz223@users.noreply.github.com> Date: Sun, 30 Aug 2026 17:03:40 +0300 Subject: [PATCH] Never wait for the cache lock in __traverse__ --- src/policies/wrapped.rs | 10 +++++ src/pyclasses/cache.rs | 4 +- src/pyclasses/fifocache.rs | 4 +- src/pyclasses/lfucache.rs | 4 +- src/pyclasses/lrucache.rs | 4 +- src/pyclasses/rrcache.rs | 4 +- src/pyclasses/ttlcache.rs | 4 +- src/pyclasses/vttlcache.rs | 4 +- tests/mixins.py | 82 ++++++++++++++++++++++++++++++++++++++ 9 files changed, 113 insertions(+), 7 deletions(-) diff --git a/src/policies/wrapped.rs b/src/policies/wrapped.rs index c4a5b32..a75a485 100644 --- a/src/policies/wrapped.rs +++ b/src/policies/wrapped.rs @@ -53,6 +53,16 @@ impl Wrapped

{ pub fn policy(&self) -> parking_lot::MutexGuard<'_, P> { self.inner.lock() } + + /// Acquires the mutex only if it is free, returning `None` otherwise. + /// + /// For callers that must never wait for the lock, such as `__traverse__`: + /// the thread holding the lock may be running Python code, and a garbage + /// collection pass landing there would deadlock the whole process. + #[inline(always)] + pub fn try_policy(&self) -> Option> { + self.inner.try_lock() + } } #[inline(always)] diff --git a/src/pyclasses/cache.rs b/src/pyclasses/cache.rs index 36e1eba..36a0ade 100644 --- a/src/pyclasses/cache.rs +++ b/src/pyclasses/cache.rs @@ -651,7 +651,9 @@ impl PyCache { } let inner = self.0.get(); - let policy = inner.policy(); + let Some(policy) = inner.try_policy() else { + return Ok(()); + }; for handle_ref in unsafe { policy.table().iter() } { let handle = unsafe { handle_ref.as_ref() }; diff --git a/src/pyclasses/fifocache.rs b/src/pyclasses/fifocache.rs index 75056c2..6a25e72 100644 --- a/src/pyclasses/fifocache.rs +++ b/src/pyclasses/fifocache.rs @@ -679,7 +679,9 @@ impl PyFIFOCache { } let inner = self.0.get(); - let policy = inner.policy(); + let Some(policy) = inner.try_policy() else { + return Ok(()); + }; for handle in policy.entries().iter() { visit.call(handle.key().as_ref())?; diff --git a/src/pyclasses/lfucache.rs b/src/pyclasses/lfucache.rs index 663a5d1..a465ecf 100644 --- a/src/pyclasses/lfucache.rs +++ b/src/pyclasses/lfucache.rs @@ -744,7 +744,9 @@ impl PyLFUCache { } let inner = self.0.get(); - let policy = inner.policy(); + let Some(policy) = inner.try_policy() else { + return Ok(()); + }; for cursor in unsafe { policy.table().iter() } { let handle = unsafe { cursor.as_ref().element() }; diff --git a/src/pyclasses/lrucache.rs b/src/pyclasses/lrucache.rs index 1954e8c..7d14895 100644 --- a/src/pyclasses/lrucache.rs +++ b/src/pyclasses/lrucache.rs @@ -725,7 +725,9 @@ impl PyLRUCache { } let inner = self.0.get(); - let policy = inner.policy(); + let Some(policy) = inner.try_policy() else { + return Ok(()); + }; for cursor in unsafe { policy.list().iter() } { let handle = unsafe { cursor.element() }; diff --git a/src/pyclasses/rrcache.rs b/src/pyclasses/rrcache.rs index 93b5eb7..f5756f0 100644 --- a/src/pyclasses/rrcache.rs +++ b/src/pyclasses/rrcache.rs @@ -671,7 +671,9 @@ impl PyRRCache { } let inner = self.0.get(); - let policy = inner.policy(); + let Some(policy) = inner.try_policy() else { + return Ok(()); + }; for handle_ref in unsafe { policy.table().iter() } { let handle = unsafe { handle_ref.as_ref() }; diff --git a/src/pyclasses/ttlcache.rs b/src/pyclasses/ttlcache.rs index 7a56372..b3dc695 100644 --- a/src/pyclasses/ttlcache.rs +++ b/src/pyclasses/ttlcache.rs @@ -816,7 +816,9 @@ impl PyTTLCache { } let inner = self.0.get(); - let policy = inner.policy(); + let Some(policy) = inner.try_policy() else { + return Ok(()); + }; for handle in policy.entries().iter() { visit.call(handle.key().as_ref())?; diff --git a/src/pyclasses/vttlcache.rs b/src/pyclasses/vttlcache.rs index 060dc8c..cb38bf8 100644 --- a/src/pyclasses/vttlcache.rs +++ b/src/pyclasses/vttlcache.rs @@ -776,7 +776,9 @@ impl PyVTTLCache { } let inner = self.0.get(); - let policy = inner.policy(); + let Some(policy) = inner.try_policy() else { + return Ok(()); + }; for cursor in unsafe { policy.table().iter() } { let handle = unsafe { cursor.as_ref().element() }; diff --git a/tests/mixins.py b/tests/mixins.py index e135f48..9583603 100644 --- a/tests/mixins.py +++ b/tests/mixins.py @@ -101,6 +101,23 @@ def test_gc_traverse_clear(self): class InsertAndGetMixin(BaseMixin): + def test_key_eq_may_trigger_the_gc(self): + # __eq__ runs in the middle of a probe, with the lock held; a deadlock + # here would keep the GIL, so the call runs in a child process + name = type(self.create_cache()).__name__ + + try: + done = subprocess.run( + [sys.executable, "-c", EQ_TRIGGERING_GC, name], + capture_output=True, + text=True, + timeout=60, + ) + except subprocess.TimeoutExpired: + pytest.fail(f"{name}.get() with a colliding key never returned") + + assert done.stdout.strip() == "ok", done.stderr + def test_insert_returns_none_on_new_key(self): cache = self.create_cache() @@ -205,6 +222,55 @@ def test_setdefault_returns_existing_value(self): assert cache.get("k") == "existing" +DROPPED_VALUE_TRIGGERING_GC = """ +import gc +import sys + +import cachebox + +name = sys.argv[1] +cls = getattr(cachebox, name) +cache = cls(10, global_ttl=60) if name == "TTLCache" else cls(10) + + +class Boom: + def __del__(self): + gc.collect() + + +cache.insert("k", Boom()) +cache.clear() +print("ok") +""" + +EQ_TRIGGERING_GC = """ +import gc +import sys + +import cachebox + +name = sys.argv[1] +cls = getattr(cachebox, name) +cache = cls(10, global_ttl=60) if name == "TTLCache" else cls(10) + + +class Key: + def __init__(self, name): + self.name = name + + def __hash__(self): + return 42 # same hash for every key, so lookups have to call __eq__ + + def __eq__(self, other): + gc.collect() + return self.name == other.name + + +cache.insert(Key("a"), 1) +assert cache.get(Key("b")) is None +print("ok") +""" + FACTORY_TOUCHING_CACHE = """ import gc import sys @@ -635,6 +701,22 @@ def test_generation_version_on_popitem(self): class DrainClearShrinkMixin(BaseMixin): + def test_dropping_a_value_may_trigger_the_gc(self): + # a deadlock here would keep the GIL, so the call runs in a child process + name = type(self.create_cache()).__name__ + + try: + done = subprocess.run( + [sys.executable, "-c", DROPPED_VALUE_TRIGGERING_GC, name], + capture_output=True, + text=True, + timeout=60, + ) + except subprocess.TimeoutExpired: + pytest.fail(f"{name}.clear() never returned") + + assert done.stdout.strip() == "ok", done.stderr + def test_clear_removes_all_items(self): cache = self.create_cache()