diff --git a/cachebox/_core.pyi b/cachebox/_core.pyi index 322b377..d99a5f0 100644 --- a/cachebox/_core.pyi +++ b/cachebox/_core.pyi @@ -384,7 +384,7 @@ class Cache(BaseCacheImpl[KT, VT]): """ Get `key`s value, or automatically create and insert one via `factory`. If `key` exists, its current value is returned and `factory` is not called. - Otherwise `factory` is called exactly once under an internal lock, its + Otherwise `factory` is called with the internal lock released, its result is inserted and returned. Args: @@ -392,8 +392,9 @@ class Cache(BaseCacheImpl[KT, VT]): factory: The factory to call and get default value from if ``key`` is not in the cache. Warning: - `factory` must not call back into this cache (deadlock risk) or block - for long. If `factory` raises, nothing is inserted and the exception + if two threads miss the same key at once, `factory` can run + more than once; the value inserted first wins and is returned to + both. If `factory` raises, nothing is inserted and the exception propagates. """ ... @@ -565,7 +566,7 @@ class FIFOCache(BaseCacheImpl[KT, VT]): """ Get `key`s value, or automatically create and insert one via `factory`. If `key` exists, its current value is returned and `factory` is not called. - Otherwise `factory` is called exactly once under an internal lock, its + Otherwise `factory` is called with the internal lock released, its result is inserted and returned. Args: @@ -573,8 +574,9 @@ class FIFOCache(BaseCacheImpl[KT, VT]): factory: The factory to call and get default value from if ``key`` is not in the cache. Warning: - `factory` must not call back into this cache (deadlock risk) or block - for long. If `factory` raises, nothing is inserted and the exception + if two threads miss the same key at once, `factory` can run + more than once; the value inserted first wins and is returned to + both. If `factory` raises, nothing is inserted and the exception propagates. """ ... @@ -779,7 +781,7 @@ class RRCache(BaseCacheImpl[KT, VT]): """ Get `key`s value, or automatically create and insert one via `factory`. If `key` exists, its current value is returned and `factory` is not called. - Otherwise `factory` is called exactly once under an internal lock, its + Otherwise `factory` is called with the internal lock released, its result is inserted and returned. Args: @@ -787,8 +789,9 @@ class RRCache(BaseCacheImpl[KT, VT]): factory: The factory to call and get default value from if ``key`` is not in the cache. Warning: - `factory` must not call back into this cache (deadlock risk) or block - for long. If `factory` raises, nothing is inserted and the exception + if two threads miss the same key at once, `factory` can run + more than once; the value inserted first wins and is returned to + both. If `factory` raises, nothing is inserted and the exception propagates. """ ... @@ -986,7 +989,7 @@ class LRUCache(BaseCacheImpl[KT, VT]): """ Get `key`s value, or automatically create and insert one via `factory`. If `key` exists, its current value is returned and `factory` is not called. - Otherwise `factory` is called exactly once under an internal lock, its + Otherwise `factory` is called with the internal lock released, its result is inserted and returned. Args: @@ -994,8 +997,9 @@ class LRUCache(BaseCacheImpl[KT, VT]): factory: The factory to call and get default value from if ``key`` is not in the cache. Warning: - `factory` must not call back into this cache (deadlock risk) or block - for long. If `factory` raises, nothing is inserted and the exception + if two threads miss the same key at once, `factory` can run + more than once; the value inserted first wins and is returned to + both. If `factory` raises, nothing is inserted and the exception propagates. """ ... @@ -1234,7 +1238,7 @@ class LFUCache(BaseCacheImpl[KT, VT]): """ Get `key`s value, or automatically create and insert one via `factory`. If `key` exists, its current value is returned and `factory` is not called. - Otherwise `factory` is called exactly once under an internal lock, its + Otherwise `factory` is called with the internal lock released, its result is inserted and returned. Args: @@ -1242,8 +1246,9 @@ class LFUCache(BaseCacheImpl[KT, VT]): factory: The factory to call and get default value from if ``key`` is not in the cache. Warning: - `factory` must not call back into this cache (deadlock risk) or block - for long. If `factory` raises, nothing is inserted and the exception + if two threads miss the same key at once, `factory` can run + more than once; the value inserted first wins and is returned to + both. If `factory` raises, nothing is inserted and the exception propagates. """ ... @@ -1441,7 +1446,7 @@ class TTLCache(BaseCacheImpl[KT, VT]): """ Get `key`s value, or automatically create and insert one via `factory`. If `key` exists, its current value is returned and `factory` is not called. - Otherwise `factory` is called exactly once under an internal lock, its + Otherwise `factory` is called with the internal lock released, its result is inserted and returned. Args: @@ -1449,8 +1454,9 @@ class TTLCache(BaseCacheImpl[KT, VT]): factory: The factory to call and get default value from if ``key`` is not in the cache. Warning: - `factory` must not call back into this cache (deadlock risk) or block - for long. If `factory` raises, nothing is inserted and the exception + if two threads miss the same key at once, `factory` can run + more than once; the value inserted first wins and is returned to + both. If `factory` raises, nothing is inserted and the exception propagates. """ ... @@ -1708,7 +1714,7 @@ class VTTLCache(BaseCacheImpl[KT, VT]): """ Get `key`s value, or automatically create and insert one via `factory`. If `key` exists, its current value is returned and `factory` is not called. - Otherwise `factory` is called exactly once under an internal lock, its + Otherwise `factory` is called with the internal lock released, its result is inserted and returned. Args: @@ -1717,8 +1723,9 @@ class VTTLCache(BaseCacheImpl[KT, VT]): ttl: An optional time-to-live duration for item. Warning: - `factory` must not call back into this cache (deadlock risk) or block - for long. If `factory` raises, nothing is inserted and the exception + if two threads miss the same key at once, `factory` can run + more than once; the value inserted first wins and is returned to + both. If `factory` raises, nothing is inserted and the exception propagates. """ ... diff --git a/src/pyclasses/cache.rs b/src/pyclasses/cache.rs index de1ef9d..c866592 100644 --- a/src/pyclasses/cache.rs +++ b/src/pyclasses/cache.rs @@ -359,11 +359,12 @@ impl PyCache { /// Get `key`s value, or automatically create and insert one via `factory`. /// /// If `key` exists, its current value is returned and `factory` is not called. - /// Otherwise `factory` is called exactly once under an internal lock, its + /// Otherwise `factory` is called with the internal lock released, its /// result is inserted and returned. /// - /// Warning: `factory` must not call back into this cache (deadlock risk) or block - /// for long. If `factory` raises, nothing is inserted and the exception + /// Warning: if two threads miss the same key at once, `factory` can run + /// more than once; the value inserted first wins and is returned to + /// both. If `factory` raises, nothing is inserted and the exception /// propagates. fn setdefault_with( &self, @@ -378,14 +379,24 @@ impl PyCache { let inner = self.0.get(); let shared = inner.shared(); + + { + let mut policy = inner.policy(); + + if let Some(x) = policy.get(py, &key)? { + return Ok(x.value().clone_ref(py)); + } + } + + // `factory` is Python code: a GC pass inside it would deadlock on `__traverse__` + let default_object = factory.call0(py)?; + let mut policy = inner.policy(); if let Some(x) = policy.get(py, &key)? { return Ok(x.value().clone_ref(py)); } - let default_object = factory.call0(py)?; - let handle = nopolicy::Handle::with_precomputed_hash_key( py, shared.getsizeof(), diff --git a/src/pyclasses/fifocache.rs b/src/pyclasses/fifocache.rs index ffdb360..16d8d6e 100644 --- a/src/pyclasses/fifocache.rs +++ b/src/pyclasses/fifocache.rs @@ -365,11 +365,12 @@ impl PyFIFOCache { /// Get `key`s value, or automatically create and insert one via `factory`. /// /// If `key` exists, its current value is returned and `factory` is not called. - /// Otherwise `factory` is called exactly once under an internal lock, its + /// Otherwise `factory` is called with the internal lock released, its /// result is inserted and returned. /// - /// Warning: `factory` must not call back into this cache (deadlock risk) or block - /// for long. If `factory` raises, nothing is inserted and the exception + /// Warning: if two threads miss the same key at once, `factory` can run + /// more than once; the value inserted first wins and is returned to + /// both. If `factory` raises, nothing is inserted and the exception /// propagates. fn setdefault_with( &self, @@ -384,14 +385,24 @@ impl PyFIFOCache { let inner = self.0.get(); let shared = inner.shared(); + + { + let mut policy = inner.policy(); + + if let Some(x) = policy.get(py, &key)? { + return Ok(x.value().clone_ref(py)); + } + } + + // `factory` is Python code: a GC pass inside it would deadlock on `__traverse__` + let default_object = factory.call0(py)?; + let mut policy = inner.policy(); if let Some(x) = policy.get(py, &key)? { return Ok(x.value().clone_ref(py)); } - let default_object = factory.call0(py)?; - let handle = fifopolicy::Handle::with_precomputed_hash_key( py, shared.getsizeof(), diff --git a/src/pyclasses/lfucache.rs b/src/pyclasses/lfucache.rs index 369e984..af80851 100644 --- a/src/pyclasses/lfucache.rs +++ b/src/pyclasses/lfucache.rs @@ -384,11 +384,12 @@ impl PyLFUCache { /// Get `key`s value, or automatically create and insert one via `factory`. /// /// If `key` exists, its current value is returned and `factory` is not called. - /// Otherwise, `factory` is called exactly once under an internal lock, its + /// Otherwise, `factory` is called with the internal lock released, its /// result is inserted and returned. /// - /// Warning: `factory` must not call back into this cache (deadlock risk) or block - /// for long. If `factory` raises, nothing is inserted and the exception + /// Warning: if two threads miss the same key at once, `factory` can run + /// more than once; the value inserted first wins and is returned to + /// both. If `factory` raises, nothing is inserted and the exception /// propagates. fn setdefault_with( &self, @@ -403,14 +404,24 @@ impl PyLFUCache { let inner = self.0.get(); let shared = inner.shared(); + + { + let mut policy = inner.policy(); + + if let Some(x) = policy.get(py, &key)? { + return Ok(x.value().clone_ref(py)); + } + } + + // `factory` is Python code: a GC pass inside it would deadlock on `__traverse__` + let default_object = factory.call0(py)?; + let mut policy = inner.policy(); if let Some(x) = policy.get(py, &key)? { return Ok(x.value().clone_ref(py)); } - let default_object = factory.call0(py)?; - let handle = lfupolicy::FrequencyHandle::with_precomputed_hash_key( py, shared.getsizeof(), diff --git a/src/pyclasses/lrucache.rs b/src/pyclasses/lrucache.rs index 1eb6a5a..f324e38 100644 --- a/src/pyclasses/lrucache.rs +++ b/src/pyclasses/lrucache.rs @@ -392,11 +392,12 @@ impl PyLRUCache { /// Get `key`s value, or automatically create and insert one via `factory`. /// /// If `key` exists, its current value is returned and `factory` is not called. - /// Otherwise `factory` is called exactly once under an internal lock, its + /// Otherwise `factory` is called with the internal lock released, its /// result is inserted and returned. /// - /// Warning: `factory` must not call back into this cache (deadlock risk) or block - /// for long. If `factory` raises, nothing is inserted and the exception + /// Warning: if two threads miss the same key at once, `factory` can run + /// more than once; the value inserted first wins and is returned to + /// both. If `factory` raises, nothing is inserted and the exception /// propagates. fn setdefault_with( &self, @@ -411,14 +412,24 @@ impl PyLRUCache { let inner = self.0.get(); let shared = inner.shared(); + + { + let mut policy = inner.policy(); + + if let Some(x) = policy.get(py, &key)? { + return Ok(x.value().clone_ref(py)); + } + } + + // `factory` is Python code: a GC pass inside it would deadlock on `__traverse__` + let default_object = factory.call0(py)?; + let mut policy = inner.policy(); if let Some(x) = policy.get(py, &key)? { return Ok(x.value().clone_ref(py)); } - let default_object = factory.call0(py)?; - let handle = lrupolicy::Handle::with_precomputed_hash_key( py, shared.getsizeof(), diff --git a/src/pyclasses/rrcache.rs b/src/pyclasses/rrcache.rs index 6d182c7..ded54d4 100644 --- a/src/pyclasses/rrcache.rs +++ b/src/pyclasses/rrcache.rs @@ -363,11 +363,12 @@ impl PyRRCache { /// Get `key`s value, or automatically create and insert one via `factory`. /// /// If `key` exists, its current value is returned and `factory` is not called. - /// Otherwise `factory` is called exactly once under an internal lock, its + /// Otherwise `factory` is called with the internal lock released, its /// result is inserted and returned. /// - /// Warning: `factory` must not call back into this cache (deadlock risk) or block - /// for long. If `factory` raises, nothing is inserted and the exception + /// Warning: if two threads miss the same key at once, `factory` can run + /// more than once; the value inserted first wins and is returned to + /// both. If `factory` raises, nothing is inserted and the exception /// propagates. fn setdefault_with( &self, @@ -382,14 +383,24 @@ impl PyRRCache { let inner = self.0.get(); let shared = inner.shared(); + + { + let mut policy = inner.policy(); + + if let Some(x) = policy.get(py, &key)? { + return Ok(x.value().clone_ref(py)); + } + } + + // `factory` is Python code: a GC pass inside it would deadlock on `__traverse__` + let default_object = factory.call0(py)?; + let mut policy = inner.policy(); if let Some(x) = policy.get(py, &key)? { return Ok(x.value().clone_ref(py)); } - let default_object = factory.call0(py)?; - let handle = rrpolicy::Handle::with_precomputed_hash_key( py, shared.getsizeof(), diff --git a/src/pyclasses/ttlcache.rs b/src/pyclasses/ttlcache.rs index 5f8cf08..e45cf6e 100644 --- a/src/pyclasses/ttlcache.rs +++ b/src/pyclasses/ttlcache.rs @@ -369,11 +369,12 @@ impl PyTTLCache { /// Get `key`s value, or automatically create and insert one via `factory`. /// /// If `key` exists, its current value is returned and `factory` is not called. - /// Otherwise, `factory` is called exactly once under an internal lock, its + /// Otherwise, `factory` is called with the internal lock released, its /// result is inserted and returned. /// - /// Warning: `factory` must not call back into this cache (deadlock risk) or block - /// for long. If `factory` raises, nothing is inserted and the exception + /// Warning: if two threads miss the same key at once, `factory` can run + /// more than once; the value inserted first wins and is returned to + /// both. If `factory` raises, nothing is inserted and the exception /// propagates. fn setdefault_with( &self, @@ -388,14 +389,24 @@ impl PyTTLCache { let inner = self.0.get(); let shared = inner.shared(); + + { + let mut policy = inner.policy(); + + if let Some(x) = policy.get(py, &key)? { + return Ok(x.value().clone_ref(py)); + } + } + + // `factory` is Python code: a GC pass inside it would deadlock on `__traverse__` + let default_object = factory.call0(py)?; + let mut policy = inner.policy(); if let Some(x) = policy.get(py, &key)? { return Ok(x.value().clone_ref(py)); } - let default_object = factory.call0(py)?; - let handle = ttlpolicy::ExpiringHandle::with_precomputed_hash_key( py, shared.getsizeof(), diff --git a/src/pyclasses/vttlcache.rs b/src/pyclasses/vttlcache.rs index 897130c..a738da5 100644 --- a/src/pyclasses/vttlcache.rs +++ b/src/pyclasses/vttlcache.rs @@ -366,14 +366,24 @@ impl PyVTTLCache { let inner = self.0.get(); let shared = inner.shared(); + + { + let mut policy = inner.policy(); + + if let Some(x) = policy.get(py, &key)? { + return Ok(x.value().clone_ref(py)); + } + } + + // `factory` is Python code: a GC pass inside it would deadlock on `__traverse__` + let default_object = factory.call0(py)?; + let mut policy = inner.policy(); if let Some(x) = policy.get(py, &key)? { return Ok(x.value().clone_ref(py)); } - let default_object = factory.call0(py)?; - let handle = vttlpolicy::ExpiringHandle::with_precomputed_hash_key( py, shared.getsizeof(), diff --git a/tests/mixins.py b/tests/mixins.py index 63c71d3..f04f8c7 100644 --- a/tests/mixins.py +++ b/tests/mixins.py @@ -2,6 +2,7 @@ import dataclasses import gc import pickle +import subprocess import sys import threading import time @@ -202,6 +203,28 @@ def test_setdefault_returns_existing_value(self): assert cache.get("k") == "existing" +FACTORY_TOUCHING_CACHE = """ +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) +cache.insert("seen", "value") + + +def factory(): + gc.collect() + return cache["seen"] + + +assert cache.setdefault_with("k", factory) == "value" +print("ok") +""" + + class SetDefaultWithMixin(BaseMixin): def test_setdefault_with_inserts_when_absent(self): cache = self.create_cache() @@ -240,6 +263,22 @@ def test_setdefault_with_exception_propagates(self): with pytest.raises(ValueError): cache.setdefault_with("k", factory) + def test_setdefault_with_factory_may_touch_the_cache_and_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", FACTORY_TOUCHING_CACHE, name], + capture_output=True, + text=True, + timeout=60, + ) + except subprocess.TimeoutExpired: + pytest.fail(f"{name}.setdefault_with never returned") + + assert done.stdout.strip() == "ok", done.stderr + class PopAndDeleteMixin(BaseMixin): def test_pop_existing_key(self):