From d4867dba8f75bf4594497f05d86618056d6391fd Mon Sep 17 00:00:00 2001 From: Roman Varnava Date: Thu, 29 Oct 2026 14:36:34 +0300 Subject: [PATCH 1/7] test: update() with an iterable that uses the cache, and two caches updated from each other --- tests/mixins.py | 94 +++++++++++++++++++++++++++++++++++++++++++++++++ 1 file changed, 94 insertions(+) diff --git a/tests/mixins.py b/tests/mixins.py index 33731cc..ac7a2a5 100644 --- a/tests/mixins.py +++ b/tests/mixins.py @@ -417,6 +417,19 @@ def test_update_overwrites_existing(self): cache.update({"a": 99}) assert cache.get("a") == 99 + def test_update_keeps_entries_read_before_an_error(self): + cache = self.create_cache() + + def pairs(): + yield "a", 1 + yield "b", 2 + raise ValueError("stop") + + with pytest.raises(ValueError, match="stop"): + cache.update(pairs()) + assert cache.get("a") == 1 + assert cache.get("b") == 2 + def test_update_invalid_argument(self): cache = self.create_cache() @@ -1142,6 +1155,64 @@ def insert(base): print("finished") """ +UPDATE_RUNNING_PYTHON_CODE = """ +import faulthandler +import sys +import threading + +import cachebox + +name = sys.argv[1] +cls = getattr(cachebox, name) +# a hang prints every thread's stack before the parent's timeout +faulthandler.dump_traceback_later(50, exit=True) +# switch threads as often as possible, also inside Key.__eq__ +sys.setswitchinterval(1e-6) + + +def make(n): + cache = cls(0, global_ttl=60) if name == "TTLCache" else cls(0) + for i in range(n): + cache[Key(i)] = i + return cache + + +class Key: + def __init__(self, v): + self.v = v + + def __hash__(self): + return self.v + + def __eq__(self, other): + return self.v == other.v + + +# the iterable reads the cache that is being updated +a = make(10) +a.update((Key(i), a.get(Key(i), 0) + 1) for i in range(20)) +assert a.get(Key(19)) == 1, a.get(Key(19)) + +# two caches updated from each other +a, b = make(200), make(200) + + +def update(x, y): + for _ in range(100): + x.update(y) + + +threads = [ + threading.Thread(target=update, args=(a, b)), + threading.Thread(target=update, args=(b, a)), +] +for t in threads: + t.start() +for t in threads: + t.join() +print("ok") +""" + LOCK_WAITERS_WITH_FINALIZERS = """ import faulthandler import random @@ -1331,6 +1402,29 @@ def test_concurrent_inserts_with_finalizers(self): done.stderr or f"exit code {done.returncode}" ) + def test_update_running_python_code(self): + # https://github.com/awolverp/cachebox/issues/118 + # + # update() iterated its argument while holding the cache lock, so an + # iterable that used the same cache hung, and so did a.update(b) with + # b.update(a) on another thread. + name = type(self.create_cache()).__name__ + + try: + done = subprocess.run( + [sys.executable, "-c", UPDATE_RUNNING_PYTHON_CODE, name], + capture_output=True, + text=True, + timeout=60, + check=False, + ) + except subprocess.TimeoutExpired: + pytest.fail(f"{name}: update() never returned") + + assert done.stdout.strip() == "ok", ( + done.stderr or f"exit code {done.returncode}" + ) + def test_lock_waiters_with_finalizers(self): # Follow-up to #95: methods such as len() and clear() waited for the # cache lock without detaching. With the GIL, a finalizer running under From 099643753b4d4c7b614435c761718e17e26fb4cc Mon Sep 17 00:00:00 2001 From: Roman Varnava Date: Thu, 29 Oct 2026 14:36:34 +0300 Subject: [PATCH 2/7] fix: build the handles for update() before taking the cache lock (#118) --- src/policies/wrapped.rs | 77 +++++++++++++++++++++++++---------------- 1 file changed, 47 insertions(+), 30 deletions(-) diff --git a/src/policies/wrapped.rs b/src/policies/wrapped.rs index badc6e9..ee6d7b8 100644 --- a/src/policies/wrapped.rs +++ b/src/policies/wrapped.rs @@ -214,6 +214,12 @@ impl Wrapped

{ /// other dict-like types; `.items()` is called and the result is iterated. /// - **Any other iterable** — iterated directly, with each element expected to /// unpack as a `(key, value)` pair. + /// + /// Iterating `iterable` and building handles runs Python code (`__iter__`, + /// `__hash__`, `getsizeof`) that may use this cache or lock another one, so it + /// all happens before the lock is taken, as in [`Self::insert`]. Entries read + /// before an error are still inserted, like `dict.update`. + // https://github.com/awolverp/cachebox/issues/118 #[inline] pub fn extend(&self, iterable: alias::BoundObject, mut transform: F) -> pyo3::PyResult<()> where @@ -222,45 +228,56 @@ impl Wrapped

{ use pyo3::types::PyAnyMethods; use pyo3::types::PyDictMethods; - let mut lock = self.policy_py_attached(iterable.py()); - - // Using [pyo3::ffi::PyObject_TypeCheck] and [Bound::cast_unchecked] is so faster than [Bound::cast] - let is_dictionary = unsafe { - pyo3::ffi::PyObject_TypeCheck(iterable.as_ptr(), crate::typeref::STD_DICT_TYPE) == 1 - }; - if is_dictionary { - let dict = unsafe { iterable.cast_unchecked::() }; - - for pair in dict.items() { - let (key, value) = unsafe { - pair.extract::<(alias::PyObject, alias::PyObject)>() - .unwrap_unchecked() - }; + let py = iterable.py(); + let mut handles = Vec::new(); + + let collected = (|| { + // Using [pyo3::ffi::PyObject_TypeCheck] and [Bound::cast_unchecked] is so faster than [Bound::cast] + let is_dictionary = unsafe { + pyo3::ffi::PyObject_TypeCheck(iterable.as_ptr(), crate::typeref::STD_DICT_TYPE) == 1 + }; + if is_dictionary { + let dict = unsafe { iterable.cast_unchecked::() }; + handles.reserve(dict.len()); + + for pair in dict.items() { + let (key, value) = unsafe { + pair.extract::<(alias::PyObject, alias::PyObject)>() + .unwrap_unchecked() + }; + + handles.push(transform(key, value)?); + } - insert_inner(&mut lock, &self.shared, pair.py(), transform(key, value)?)?; + return Ok(()); } - return Ok(()); - } + // By this we will support everything has `.items()` attribute, + // including our cache classes + let items_iterable = { + if let Some(items_attribute) = iterable.getattr_opt(c"items")? { + items_attribute.call0()? + } else { + iterable + } + }; - // By this we will support everything has `.items()` attribute, - // including our cache classes - let items_iterable = { - if let Some(items_attribute) = iterable.getattr_opt(c"items")? { - items_attribute.call0()? - } else { - iterable + for pair in items_iterable.try_iter()? { + let pair = pair?; + let (key, value) = pair.extract::<(alias::PyObject, alias::PyObject)>()?; + + handles.push(transform(key, value)?); } - }; - for pair in items_iterable.try_iter()? { - let pair = pair?; - let (key, value) = pair.extract::<(alias::PyObject, alias::PyObject)>()?; + Ok(()) + })(); - insert_inner(&mut lock, &self.shared, pair.py(), transform(key, value)?)?; + let mut lock = self.policy_py_attached(py); + for handle in handles { + insert_inner(&mut lock, &self.shared, py, handle)?; } - Ok(()) + collected } /// Calls the `evict()` `n` times and returns count of removed items. From 3d1975d7f36c09baa1aaee73cecf4c4c657552bc Mon Sep 17 00:00:00 2001 From: Roman Varnava Date: Thu, 29 Oct 2026 14:40:42 +0300 Subject: [PATCH 3/7] perf: reserve the update() buffer from len() when the iterable has one --- src/policies/wrapped.rs | 4 ++++ tests/mixins.py | 13 +++++++++++++ 2 files changed, 17 insertions(+) diff --git a/src/policies/wrapped.rs b/src/policies/wrapped.rs index ee6d7b8..ee1a198 100644 --- a/src/policies/wrapped.rs +++ b/src/policies/wrapped.rs @@ -262,6 +262,10 @@ impl Wrapped

{ } }; + // Only a hint: a generator has no len() (its TypeError is dropped here), + // and a wrong __len__ must not make the reservation abort. + let _ = handles.try_reserve(items_iterable.len().unwrap_or(0)); + for pair in items_iterable.try_iter()? { let pair = pair?; let (key, value) = pair.extract::<(alias::PyObject, alias::PyObject)>()?; diff --git a/tests/mixins.py b/tests/mixins.py index ac7a2a5..586ab5e 100644 --- a/tests/mixins.py +++ b/tests/mixins.py @@ -430,6 +430,19 @@ def pairs(): assert cache.get("a") == 1 assert cache.get("b") == 2 + def test_update_ignores_a_wrong_len(self): + class Lying: + def __len__(self): + return 2**62 + + def __iter__(self): + return iter([("a", 1)]) + + cache = self.create_cache() + + cache.update(Lying()) + assert cache.get("a") == 1 + def test_update_invalid_argument(self): cache = self.create_cache() From 449fd8a37b051288e148da6c9f45eaa21f262f23 Mon Sep 17 00:00:00 2001 From: Roman Varnava Date: Thu, 29 Oct 2026 14:45:39 +0300 Subject: [PATCH 4/7] fix: insert the update() input in chunks so memory follows the cache size --- src/policies/wrapped.rs | 38 ++++++++++++++++++++++++++++---------- tests/mixins.py | 20 ++++++++++++++++++++ 2 files changed, 48 insertions(+), 10 deletions(-) diff --git a/src/policies/wrapped.rs b/src/policies/wrapped.rs index ee1a198..732c0f6 100644 --- a/src/policies/wrapped.rs +++ b/src/policies/wrapped.rs @@ -93,6 +93,10 @@ impl Wrapped

{ } } +/// How many handles [`Wrapped::extend`] builds before inserting them, which +/// bounds its memory when the input is much larger than the cache. +const EXTEND_CHUNK: usize = 1024; + #[inline(always)] fn insert_inner( lock: &mut parking_lot::MutexGuard<'_, P>, @@ -217,8 +221,9 @@ impl Wrapped

{ /// /// Iterating `iterable` and building handles runs Python code (`__iter__`, /// `__hash__`, `getsizeof`) that may use this cache or lock another one, so it - /// all happens before the lock is taken, as in [`Self::insert`]. Entries read - /// before an error are still inserted, like `dict.update`. + /// never runs under the lock, as in [`Self::insert`]: handles are built in + /// chunks of [`EXTEND_CHUNK`] and each chunk is inserted under one lock. + /// Entries read before an error are still inserted, like `dict.update`. // https://github.com/awolverp/cachebox/issues/118 #[inline] pub fn extend(&self, iterable: alias::BoundObject, mut transform: F) -> pyo3::PyResult<()> @@ -238,7 +243,7 @@ impl Wrapped

{ }; if is_dictionary { let dict = unsafe { iterable.cast_unchecked::() }; - handles.reserve(dict.len()); + handles.reserve(dict.len().min(EXTEND_CHUNK)); for pair in dict.items() { let (key, value) = unsafe { @@ -247,6 +252,9 @@ impl Wrapped

{ }; handles.push(transform(key, value)?); + if handles.len() == EXTEND_CHUNK { + self.insert_all(py, &mut handles)?; + } } return Ok(()); @@ -262,28 +270,38 @@ impl Wrapped

{ } }; - // Only a hint: a generator has no len() (its TypeError is dropped here), - // and a wrong __len__ must not make the reservation abort. - let _ = handles.try_reserve(items_iterable.len().unwrap_or(0)); + // Only a hint: a generator has no len() (its TypeError is dropped here). + handles.reserve(items_iterable.len().unwrap_or(0).min(EXTEND_CHUNK)); for pair in items_iterable.try_iter()? { let pair = pair?; let (key, value) = pair.extract::<(alias::PyObject, alias::PyObject)>()?; handles.push(transform(key, value)?); + if handles.len() == EXTEND_CHUNK { + self.insert_all(py, &mut handles)?; + } } Ok(()) })(); - let mut lock = self.policy_py_attached(py); - for handle in handles { - insert_inner(&mut lock, &self.shared, py, handle)?; + if !handles.is_empty() { + self.insert_all(py, &mut handles)?; } - collected } + /// Inserts `handles` under one lock, leaving `handles` empty. + fn insert_all(&self, py: pyo3::Python<'_>, handles: &mut Vec) -> pyo3::PyResult<()> { + // Declared before the lock, so after an error the handles not inserted + // are dropped (and their finalizers run) once the lock is released. + let mut pending = handles.drain(..); + let mut lock = self.policy_py_attached(py); + + pending.try_for_each(|handle| insert_inner(&mut lock, &self.shared, py, handle).map(drop)) + } + /// Calls the `evict()` `n` times and returns count of removed items. #[inline] pub fn drain( diff --git a/tests/mixins.py b/tests/mixins.py index 586ab5e..b0c96e9 100644 --- a/tests/mixins.py +++ b/tests/mixins.py @@ -8,6 +8,7 @@ import sysconfig import threading import time +import tracemalloc import typing import weakref from datetime import timedelta @@ -430,6 +431,25 @@ def pairs(): assert cache.get("a") == 1 assert cache.get("b") == 2 + @pytest.mark.skipif( + platform.python_implementation() == "PyPy", + reason="PyPy has no tracemalloc", + ) + def test_update_memory_follows_the_cache_not_the_input(self): + cache = self.create_cache(10) + if isinstance(cache, cachebox.Cache): + pytest.skip("Cache does not evict") + + tracemalloc.start() + try: + cache.update((i, bytes(1000)) for i in range(20_000)) + _, peak = tracemalloc.get_traced_memory() + finally: + tracemalloc.stop() + + # all 20k values alive at once would be about 20 MB + assert peak < 5_000_000, peak + def test_update_ignores_a_wrong_len(self): class Lying: def __len__(self): From 4ad9c7faa36ff033a7457e280113de3db865fb50 Mon Sep 17 00:00:00 2001 From: Roman Varnava Date: Thu, 29 Oct 2026 14:53:13 +0300 Subject: [PATCH 5/7] test: import tracemalloc inside the memory test (PyPy has none) --- tests/mixins.py | 6 +----- 1 file changed, 1 insertion(+), 5 deletions(-) diff --git a/tests/mixins.py b/tests/mixins.py index b0c96e9..babe393 100644 --- a/tests/mixins.py +++ b/tests/mixins.py @@ -8,7 +8,6 @@ import sysconfig import threading import time -import tracemalloc import typing import weakref from datetime import timedelta @@ -431,11 +430,8 @@ def pairs(): assert cache.get("a") == 1 assert cache.get("b") == 2 - @pytest.mark.skipif( - platform.python_implementation() == "PyPy", - reason="PyPy has no tracemalloc", - ) def test_update_memory_follows_the_cache_not_the_input(self): + tracemalloc = pytest.importorskip("tracemalloc") # not on PyPy cache = self.create_cache(10) if isinstance(cache, cachebox.Cache): pytest.skip("Cache does not evict") From 7fdbf4c8c7872cc303da1801fed76b587e4b199c Mon Sep 17 00:00:00 2001 From: Roman Varnava Date: Thu, 29 Oct 2026 15:26:29 +0300 Subject: [PATCH 6/7] perf: keep the single-pass update() for dict, list, tuple and set --- src/policies/wrapped.rs | 72 +++++++++++++++++++++++++---------------- tests/mixins.py | 16 +++++++++ 2 files changed, 61 insertions(+), 27 deletions(-) diff --git a/src/policies/wrapped.rs b/src/policies/wrapped.rs index 732c0f6..f9da5ec 100644 --- a/src/policies/wrapped.rs +++ b/src/policies/wrapped.rs @@ -219,11 +219,12 @@ impl Wrapped

{ /// - **Any other iterable** — iterated directly, with each element expected to /// unpack as a `(key, value)` pair. /// - /// Iterating `iterable` and building handles runs Python code (`__iter__`, - /// `__hash__`, `getsizeof`) that may use this cache or lock another one, so it - /// never runs under the lock, as in [`Self::insert`]: handles are built in - /// chunks of [`EXTEND_CHUNK`] and each chunk is inserted under one lock. - /// Entries read before an error are still inserted, like `dict.update`. + /// A `dict`, `list`, `tuple`, `set` or `frozenset` is inserted in one pass under + /// one lock: iterating it runs no Python code. Any other iterable may run Python + /// code that uses this cache or locks another one (a generator, another + /// cache's `.items()`), so it is never iterated under the lock: handles are + /// built in chunks of [`EXTEND_CHUNK`] and each chunk is inserted under one + /// lock. Entries read before an error are still inserted, like `dict.update`. // https://github.com/awolverp/cachebox/issues/118 #[inline] pub fn extend(&self, iterable: alias::BoundObject, mut transform: F) -> pyo3::PyResult<()> @@ -234,32 +235,49 @@ impl Wrapped

{ use pyo3::types::PyDictMethods; let py = iterable.py(); - let mut handles = Vec::new(); - let collected = (|| { - // Using [pyo3::ffi::PyObject_TypeCheck] and [Bound::cast_unchecked] is so faster than [Bound::cast] - let is_dictionary = unsafe { - pyo3::ffi::PyObject_TypeCheck(iterable.as_ptr(), crate::typeref::STD_DICT_TYPE) == 1 - }; - if is_dictionary { - let dict = unsafe { iterable.cast_unchecked::() }; - handles.reserve(dict.len().min(EXTEND_CHUNK)); - - for pair in dict.items() { - let (key, value) = unsafe { - pair.extract::<(alias::PyObject, alias::PyObject)>() - .unwrap_unchecked() - }; - - handles.push(transform(key, value)?); - if handles.len() == EXTEND_CHUNK { - self.insert_all(py, &mut handles)?; - } - } + // Using [pyo3::ffi::PyObject_TypeCheck] and [Bound::cast_unchecked] is so faster than [Bound::cast] + let is_dictionary = unsafe { + pyo3::ffi::PyObject_TypeCheck(iterable.as_ptr(), crate::typeref::STD_DICT_TYPE) == 1 + }; + if is_dictionary { + let dict = unsafe { iterable.cast_unchecked::() }; + let mut lock = self.policy_py_attached(py); + + for pair in dict.items() { + let (key, value) = unsafe { + pair.extract::<(alias::PyObject, alias::PyObject)>() + .unwrap_unchecked() + }; + + insert_inner(&mut lock, &self.shared, py, transform(key, value)?)?; + } + + return Ok(()); + } + + // Iterating these built-in containers runs no Python code either (a + // subclass could override `__iter__`, hence the exact check). + if iterable.is_exact_instance_of::() + || iterable.is_exact_instance_of::() + || iterable.is_exact_instance_of::() + || iterable.is_exact_instance_of::() + { + let mut lock = self.policy_py_attached(py); - return Ok(()); + for pair in iterable.try_iter()? { + let pair = pair?; + let (key, value) = pair.extract::<(alias::PyObject, alias::PyObject)>()?; + + insert_inner(&mut lock, &self.shared, py, transform(key, value)?)?; } + return Ok(()); + } + + let mut handles = Vec::new(); + + let collected = (|| { // By this we will support everything has `.items()` attribute, // including our cache classes let items_iterable = { diff --git a/tests/mixins.py b/tests/mixins.py index babe393..94bc495 100644 --- a/tests/mixins.py +++ b/tests/mixins.py @@ -417,6 +417,22 @@ def test_update_overwrites_existing(self): cache.update({"a": 99}) assert cache.get("a") == 99 + def test_update_from_tuple_set_and_list_subclass(self): + cache = self.create_cache() + + cache.update((("t", 1),)) + cache.update({("s", 2)}) + cache.update(frozenset({("f", 3)})) + + class Pairs(list): + # a subclass may run Python code (even this cache) while iterated + def __iter__(self): + assert len(cache) >= 0 + return super().__iter__() + + cache.update(Pairs([("l", 4)])) + assert [cache.get(k) for k in "tsfl"] == [1, 2, 3, 4] + def test_update_keeps_entries_read_before_an_error(self): cache = self.create_cache() From e08bbc31858db4db648f97d887556be45f7b47b8 Mon Sep 17 00:00:00 2001 From: Roman Varnava Date: Thu, 29 Oct 2026 15:34:49 +0300 Subject: [PATCH 7/7] refactor: name the built-in container check in extend --- src/policies/wrapped.rs | 23 ++++++++++++++++------- 1 file changed, 16 insertions(+), 7 deletions(-) diff --git a/src/policies/wrapped.rs b/src/policies/wrapped.rs index f9da5ec..7eabc6a 100644 --- a/src/policies/wrapped.rs +++ b/src/policies/wrapped.rs @@ -97,6 +97,21 @@ impl Wrapped

{ /// bounds its memory when the input is much larger than the cache. const EXTEND_CHUNK: usize = 1024; +/// Built-in containers whose iteration runs no Python code. Exact types only: +/// a subclass can override `__iter__`. +fn iterates_without_python(obj: &alias::BoundObject) -> bool { + use pyo3::types::PyAnyMethods; + use pyo3::types::PyFrozenSet; + use pyo3::types::PyList; + use pyo3::types::PySet; + use pyo3::types::PyTuple; + + obj.is_exact_instance_of::() + || obj.is_exact_instance_of::() + || obj.is_exact_instance_of::() + || obj.is_exact_instance_of::() +} + #[inline(always)] fn insert_inner( lock: &mut parking_lot::MutexGuard<'_, P>, @@ -256,13 +271,7 @@ impl Wrapped

{ return Ok(()); } - // Iterating these built-in containers runs no Python code either (a - // subclass could override `__iter__`, hence the exact check). - if iterable.is_exact_instance_of::() - || iterable.is_exact_instance_of::() - || iterable.is_exact_instance_of::() - || iterable.is_exact_instance_of::() - { + if iterates_without_python(&iterable) { let mut lock = self.policy_py_attached(py); for pair in iterable.try_iter()? {