From d3ff1eb5ea657cd385a0eca90f8bcd84b68e015c Mon Sep 17 00:00:00 2001 From: youknowdot Date: Mon, 5 Oct 2026 04:48:57 -0700 Subject: [PATCH] Keep module cache entries present during import reordering (#8948) * Reorder the module cache atomically during imports Add a private exact-dict move primitive and importlib dispatcher so shutdown-order updates never temporarily remove a module. Preserve current values and custom-mapping fallback behavior; validate reentrant equality probe witnesses, invalidate layout caches, and compact moved entries. Cover all four existing bootstrap reorder sites, including legacy-loader cleanup. Add deterministic trace-window, loader replacement/removal, reentrancy, cache, iterator and compaction regressions. The private native primitive uses true-move semantics; it does not promise arbitrary pop/set callback equivalence. Assisted-by: Codex:model-version-unavailable * Keep atomic import regression compatible with modern bootstrap Run legacy-loader cases when the bootstrap exposes that helper, preserving all four 3.14 paths and the two remaining 3.15 paths. Always retain modern load and exec regressions. Assisted-by: Codex:model-version-unavailable * Keep the copied bootstrap unchanged in the native relocation PR Restore the exact CPython 3.14 bootstrap and retain only the private exact-dict relocation primitive and its direct native regressions. Remove the PR-added callback integration tests while preserving native helper assertions. This prepares a primitive only; imports do not call it and the import race remains unresolved. Assisted-by: Codex:model-version-unavailable --- crates/vm/src/builtins/dict.rs | 10 + crates/vm/src/dict_inner.rs | 236 ++++++++++++++++++ crates/vm/src/stdlib/_imp.rs | 14 +- extra_tests/snippets/import_atomic_reorder.py | 189 ++++++++++++++ 4 files changed, 447 insertions(+), 2 deletions(-) create mode 100644 extra_tests/snippets/import_atomic_reorder.py diff --git a/crates/vm/src/builtins/dict.rs b/crates/vm/src/builtins/dict.rs index d709521ea..2cc9034b1 100644 --- a/crates/vm/src/builtins/dict.rs +++ b/crates/vm/src/builtins/dict.rs @@ -1085,6 +1085,16 @@ impl Py { } impl PyExact { + pub(crate) fn move_to_end( + &self, + key: PyObjectRef, + vm: &VirtualMachine, + ) -> PyResult { + self.entries + .move_to_end(vm, &*key)? + .ok_or_else(|| vm.new_key_error(key)) + } + /// Look up `key` in `self`, falling back to `other`. /// Both dicts must be exact `dict` types (enforced by `PyExact`). pub(crate) fn get_chain_exact( diff --git a/crates/vm/src/dict_inner.rs b/crates/vm/src/dict_inner.rs index 4a901bfbf..ed6a23005 100644 --- a/crates/vm/src/dict_inner.rs +++ b/crates/vm/src/dict_inner.rs @@ -1392,6 +1392,134 @@ impl Dict { Ok(removed) } + /// Move an existing entry to the end and return its current value without + /// making the key temporarily absent. Hashing and equality run unlocked. + pub(crate) fn move_to_end( + &self, + vm: &VirtualMachine, + key: &K, + ) -> PyResult> { + struct ProbeWitness { + index_index: IndexIndex, + entry_index: IndexEntry, + key: Option<(HashValue, PyObjectRef)>, + } + + let hash_value = key.key_hash(vm)?; + let mut idxs = None; + let mut prefix = Vec::::new(); + let mut compared: Option<(EntryIndex, IndexIndex, PyObjectRef, bool)> = None; + 'lookup: loop { + // Keep the compared key alive until after the guard is released, + // including when equality cleared the dictionary and recycled its + // entry and bucket for a different key. + let comparison = compared.take(); + let mut inner = self.write(); + let mask = (inner.indices.len() - 1) as i64; + let probes = idxs.get_or_insert_with(|| GenIndexes::new(hash_value, mask)); + if probes.mask != mask { + drop(inner); + prefix.clear(); + idxs = None; + continue; + } + let matched = if let Some((entry_index, index_index, candidate, equal)) = &comparison { + let valid = inner.indices.get(*index_index).and_then(|i| i.index()) + == Some(*entry_index) + && inner + .get_entry_checked(*entry_index, *index_index) + .is_some_and(|entry| entry.hash == hash_value && entry.key.is(candidate)); + // A false comparison can survive while another previously + // probed bucket changes (including after nested compaction). + // Revalidate the whole prefix before continuing past it. + let prefix_valid = *equal + || prefix.iter().all(|witness| { + inner.indices.get(witness.index_index) == Some(&witness.entry_index) + && witness.key.as_ref().is_none_or(|(hash, key)| { + inner + .get_entry_checked( + witness.entry_index.index().unwrap(), + witness.index_index, + ) + .is_some_and(|entry| entry.hash == *hash && entry.key.is(key)) + }) + }); + if !valid || !prefix_valid { + drop(inner); + prefix.clear(); + idxs = None; + continue; + } + equal.then_some(*entry_index) + } else { + None + }; + let entry_index = match matched { + Some(index) => index, + None => loop { + let index_index = probes.next(); + let index_entry = inner.indices[index_index]; + match index_entry { + IndexEntry::FREE => return Ok(None), + IndexEntry::DUMMY => { + prefix.push(ProbeWitness { + index_index, + entry_index: index_entry, + key: None, + }); + continue; + } + _ => {} + } + let entry_index = index_entry.index().unwrap(); + let entry = inner.entries[entry_index].as_ref().unwrap(); + if key.key_is(&entry.key) { + break entry_index; + } + prefix.push(ProbeWitness { + index_index, + entry_index: index_entry, + key: Some((entry.hash, entry.key.clone())), + }); + if entry.hash == hash_value { + let candidate = entry.key.clone(); + drop(inner); + drop(comparison); + let equal = key.key_eq(vm, &candidate)?; + compared = Some((entry_index, index_index, candidate, equal)); + continue 'lookup; + } + }, + }; + let value = inner.entries[entry_index].as_ref().unwrap().value.clone(); + if inner.entries[entry_index + 1..].iter().any(Option::is_some) { + // Reserve before taking the entry. No reader can observe an + // absent entry, and no hashing, equality or finalizer runs in + // this critical section. + inner.entries.reserve(1); + self.invalidate_keys_version(); + let entry = inner.entries[entry_index].take().unwrap(); + let new_index = inner.entries.len(); + inner.indices[entry.index] = unsafe { + // SAFETY: new_index is a valid nonnegative entry index. + IndexEntry::from_index_unchecked(new_index) + }; + inner.entries.push(Some(entry)); + let holes = inner.entries.len() - inner.used; + if holes >= 2 && holes >= inner.used { + // Moves do not increase `filled`, so ordinary insertion's + // resize threshold cannot bound their accumulated holes. + // Two holes also ensure this operation changes DictSize, + // allowing existing iterators to detect the relocation. + let indices_size = inner.indices.len(); + inner.resize(indices_size); + } + } + drop(inner); + return Ok(Some(value)); + } + } + pub(crate) fn pop_back(&self) -> Option<(PyObjectRef, T)> { let inner = &mut *self.write(); let entry = loop { @@ -1838,6 +1966,114 @@ mod tests { use super::*; use crate::{Interpreter, common::ascii}; + #[test] + fn move_to_end_preserves_values_and_invalidates_layout() { + Interpreter::without_stdlib(Default::default()).enter(|vm| { + let dict = Dict::default(); + let keys = [ + vm.ctx.intern_str("first"), + vm.ctx.intern_str("middle"), + vm.ctx.intern_str("last"), + ]; + for (value, key) in keys.iter().enumerate() { + dict.insert(vm, *key, value).unwrap(); + } + let hint = dict.hint_for_key(vm, keys[1]).unwrap().unwrap() as usize; + let version = dict.assign_keys_version(); + let size = dict.size(); + assert_ne!(version, 0); + assert_eq!(dict.move_to_end(vm, keys[1]).unwrap(), Some(1)); + assert_eq!(dict.values(), vec![0, 2, 1]); + assert_eq!(dict.keys_version(), 0); + assert_eq!(dict.get_index_if_keys_version(version, hint), None); + assert_eq!(dict.get_hint(vm, keys[1], hint).unwrap(), None); + assert!(dict.next_entry_checked(0, &size, |_, v| *v).is_err()); + assert!( + dict.prev_entry_checked(size.entries_size - 1, &size, |_, v| *v) + .is_err() + ); + + let version = dict.assign_keys_version(); + let size = dict.size(); + assert_eq!(dict.move_to_end(vm, keys[1]).unwrap(), Some(1)); + assert_eq!(dict.keys_version(), version); + assert_eq!(dict.size(), size); + assert_eq!(dict.move_to_end(vm, "absent").unwrap(), None); + assert_eq!(dict.keys_version(), version); + assert_eq!(dict.size(), size); + assert_eq!(dict.values(), vec![0, 2, 1]); + }); + } + + #[test] + fn move_to_end_compacts_and_keeps_indices_consistent() { + Interpreter::without_stdlib(Default::default()).enter(|vm| { + for count in [2, 3, 32] { + let dict = Dict::default(); + for key in 0..count { + dict.insert(vm, &key, key).unwrap(); + } + for step in 0..1024 { + let key = step % count; + let size = dict.size(); + let hint = dict.hint_for_key(vm, &key).unwrap().unwrap() as usize; + let version = dict.assign_keys_version(); + assert_eq!(dict.move_to_end(vm, &key).unwrap(), Some(key)); + assert_eq!(dict.get_index_if_keys_version(version, hint), None); + assert!(dict.next_entry_checked(0, &size, |_, v| *v).is_err()); + assert!( + dict.prev_entry_checked(size.entries_size - 1, &size, |_, v| *v) + .is_err() + ); + let inner = dict.read(); + assert_eq!(inner.used, count); + assert_eq!(inner.filled, count); + assert!(inner.entries.len() < 2 * count); + assert!(inner.entries.capacity() <= 4 * count); + let values: Vec<_> = inner.entries.iter().flatten().map(|e| e.value).collect(); + assert_eq!( + values, + (1..=count).map(|i| (key + i) % count).collect::>() + ); + for (index, entry) in inner.entries.iter().enumerate() { + if let Some(entry) = entry { + assert_eq!(inner.indices[entry.index].index(), Some(index)); + assert_eq!(entry.hash, entry.value.key_hash(vm).unwrap()); + } + } + for (bucket, index) in inner.indices.iter().enumerate() { + if let Some(index) = index.index() { + assert_eq!(inner.entries[index].as_ref().unwrap().index, bucket); + } + } + } + } + }); + } + + #[test] + fn move_to_end_restores_only_matching_shared_shapes() { + Interpreter::without_stdlib(Default::default()).enter(|vm| { + let first = vm.ctx.intern_str("move_first"); + let last = vm.ctx.intern_str("move_last"); + let dict = Dict::default(); + let peer = Dict::default(); + for table in [&dict, &peer] { + table.insert(vm, first, 1).unwrap(); + table.insert(vm, last, 2).unwrap(); + } + let version = dict.assign_keys_version(); + assert_eq!(peer.assign_keys_version(), version); + dict.move_to_end(vm, first).unwrap(); + assert_ne!(dict.assign_keys_version(), version); + dict.move_to_end(vm, last).unwrap(); + // The second move compacts back to the original hole-free layout. + assert_eq!(dict.assign_keys_version(), version); + assert_eq!(dict.get_index_if_keys_version(version, 0), Some(1)); + assert_eq!(dict.get_index_if_keys_version(version, 1), Some(2)); + }); + } + #[test] fn clone_compacts_deleted_entries() { Interpreter::without_stdlib(Default::default()).enter(|vm| { diff --git a/crates/vm/src/stdlib/_imp.rs b/crates/vm/src/stdlib/_imp.rs index b2b278311..07503ea91 100644 --- a/crates/vm/src/stdlib/_imp.rs +++ b/crates/vm/src/stdlib/_imp.rs @@ -193,13 +193,23 @@ fn find_frozen(name: &str, vm: &VirtualMachine) -> Result, + key: PyObjectRef, + vm: &VirtualMachine, + ) -> PyResult { + modules.move_to_end(key, vm) + } + #[pyattr] fn check_hash_based_pycs(vm: &VirtualMachine) -> PyStrRef { vm.ctx diff --git a/extra_tests/snippets/import_atomic_reorder.py b/extra_tests/snippets/import_atomic_reorder.py new file mode 100644 index 000000000..b7c8a9d35 --- /dev/null +++ b/extra_tests/snippets/import_atomic_reorder.py @@ -0,0 +1,189 @@ +"""Private exact-dict relocation and callback-reentry regression coverage.""" + +import _imp +import sys + + +def raises(kind, call, expected=None): + try: + call() + except kind as exc: + if expected is not None: + assert exc is expected + return exc + raise AssertionError(f"{kind.__name__} was not raised") + + +def check_native_helper(): + move = _imp._dict_move_to_end + first, middle, last = object(), object(), object() + mapping = {"first": first, "middle": middle, "last": last} + assert move(mapping, "middle") is middle + assert list(mapping) == ["first", "last", "middle"] + assert len(mapping) == 3 + assert move(mapping, "middle") is middle + assert list(mapping) == ["first", "last", "middle"] + single = {"only": None} + assert move(single, "only") is None + assert single == {"only": None} + missing = object() + exc = raises(KeyError, lambda: move(mapping, missing)) + assert exc.args[0] is missing + assert list(mapping) == ["first", "last", "middle"] + + class Subclass(dict): + pass + + raises(TypeError, lambda: move(Subclass(mapping), "middle")) + raises(TypeError, lambda: move([], "middle")) + stored = "".join(["equal", " key"]) + equal = "".join(["equal ", "key"]) + assert stored == equal and stored is not equal + mapping = {stored: middle, "after": last} + assert move(mapping, equal) is middle + assert list(mapping)[-1] is stored + + failure = ValueError("hash failure") + + class Hashed(str): + count = 0 + fail = False + + def __hash__(self): + self.count += 1 + if self.fail: + raise failure + return super().__hash__() + + lookup = Hashed(stored) + assert move(mapping, lookup) is middle + assert lookup.count == 1 + lookup.fail = True + raises(ValueError, lambda: move(mapping, lookup), failure) + assert lookup.count == 2 + + class Collision: + def __init__(self, label, action=None): + self.label = label + self.action = action + + def __hash__(self): + return 42 + + def __eq__(self, other): + action, self.action = self.action, None + if action is not None: + action() + return isinstance(other, Collision) and self.label == other.label + + lookup = Collision("target") + stored = Collision("target") + impostor = Collision("different") + mapping = {stored: first, "after": last} + + def replace_key(): + mapping.clear() + mapping[impostor] = middle + + stored.action = replace_key + raises(KeyError, lambda: move(mapping, lookup)) + assert list(mapping) == [impostor] + assert mapping[impostor] is middle + + stored = Collision("target") + mapping = {stored: first, "after": last} + stored.action = lambda: mapping.__setitem__(stored, middle) + assert move(mapping, lookup) is middle + assert list(mapping)[-1] is stored + + stored = Collision("target") + mapping = {stored: first, "after": last} + stored.action = lambda: mapping.clear() + raises(KeyError, lambda: move(mapping, lookup)) + assert mapping == {} + + stored = Collision("target") + mapping = {stored: first, "after": last} + + def resize(): + mapping.clear() + for i in range(100): + mapping[i] = i + mapping[stored] = middle + mapping["after"] = last + + stored.action = resize + assert move(mapping, lookup) is middle + assert list(mapping)[-1] is stored + assert len(mapping) == 102 + + # A surviving false candidate alone does not certify the probe prefix. + # Nested relocation compacts T into P's earlier bucket, while A keeps its + # key identity, bucket and physical entry index. T never becomes absent. + prefix_key = Collision("prefix") + candidate = Collision("candidate") + target = Collision("target") + mapping = {prefix_key: first, candidate: last, target: middle} + + def compact_prefix(): + del mapping[prefix_key] + assert move(mapping, candidate) is last + assert list(mapping) == [target, candidate] + + candidate.action = compact_prefix + assert move(mapping, lookup) is middle + assert list(mapping) == [candidate, target] + + # Inserting in a previously visited DUMMY bucket also requires a restart. + mapping = {prefix_key: first, candidate: last} + del mapping[prefix_key] + candidate.action = lambda: mapping.__setitem__(lookup, middle) + assert move(mapping, lookup) is middle + assert list(mapping) == [candidate, lookup] + + failure = RuntimeError("equality failure") + + def fail(): + raise failure + + stored = Collision("target", fail) + mapping = {stored: first, "after": last} + before = list(mapping) + raises(RuntimeError, lambda: move(mapping, lookup), failure) + assert list(mapping) == before + + class HashMutation: + def __hash__(self): + mapping.clear() + mapping["kept"] = last + return 42 + + raises(KeyError, lambda: move(mapping, HashMutation())) + assert mapping == {"kept": last} + + # Releasing the removed candidate must not run its finalizer under the + # dictionary lock. The only owners are the dict and the lookup's witness. + finalized = [] + + class Finalized(Collision): + def __del__(self): + mapping["finalized"] = True + finalized.append(True) + + mapping = {Finalized("target", lambda: mapping.clear()): first} + raises(KeyError, lambda: move(mapping, lookup)) + assert finalized == [True] + assert mapping == {"finalized": True} + + mapping = {"a": 1, "b": 2} + for i in range(100): + forward, backward = iter(mapping), reversed(mapping) + key = "a" if i % 2 == 0 else "b" + move(mapping, key) + raises(RuntimeError, lambda: next(forward)) + raises(RuntimeError, lambda: next(backward)) + assert list(mapping)[-1] == key + + +if sys.implementation.name == "rustpython": + check_native_helper()