From b6e7d71e2ea9e0d075a4e4f834af65b9623d59f3 Mon Sep 17 00:00:00 2001 From: "Jeong, YunWon" Date: Mon, 9 Feb 2026 17:14:27 +0900 Subject: [PATCH] Remove static cache for builtins iter/reversed reduce - Replace static_cell! cached builtins_iter/builtins_reversed with per-call vm.builtins.get_attr lookups to support runtime builtins mutation (CPython issue #101765) - Refactor PositionIterInternal::reduce to take func, active, and empty callbacks; resolve builtins outside the lock to prevent deadlock from reentrant iterator access - Add __reduce__ and fix reentrant next() in PyCallableIterator - Remove expectedFailure from test_reverse_iterator_picking and test_reduce_mutating_builtins_iter --- Lib/test/test_array.py | 2 - Lib/test/test_iter.py | 1 - crates/stdlib/src/array.rs | 12 ++-- crates/vm/src/builtins/bytearray.rs | 12 ++-- crates/vm/src/builtins/bytes.rs | 12 ++-- crates/vm/src/builtins/dict.rs | 4 +- crates/vm/src/builtins/enumerate.rs | 11 ++- crates/vm/src/builtins/iter.rs | 108 ++++++++++++++++------------ crates/vm/src/builtins/list.rs | 25 +++++-- crates/vm/src/builtins/memory.rs | 12 ++-- crates/vm/src/builtins/range.rs | 2 +- crates/vm/src/builtins/set.rs | 2 +- crates/vm/src/builtins/str.rs | 16 +++-- crates/vm/src/builtins/tuple.rs | 14 ++-- 14 files changed, 145 insertions(+), 88 deletions(-) diff --git a/Lib/test/test_array.py b/Lib/test/test_array.py index 0376d7ff9..d9928937e 100644 --- a/Lib/test/test_array.py +++ b/Lib/test/test_array.py @@ -355,8 +355,6 @@ class BaseTest: self.assertEqual(list(a), list(self.example)) self.assertEqual(list(reversed(a)), list(iter(a))[::-1]) - # TODO: RUSTPYTHON - @unittest.expectedFailure def test_reverse_iterator_picking(self): orig = array.array(self.typecode, self.example) data = list(orig) diff --git a/Lib/test/test_iter.py b/Lib/test/test_iter.py index dfb571445..9c26eb085 100644 --- a/Lib/test/test_iter.py +++ b/Lib/test/test_iter.py @@ -249,7 +249,6 @@ class TestCase(unittest.TestCase): self.assertEqual(list(empit), [5, 6]) self.assertEqual(list(a), [0, 1, 2, 3, 4, 5, 6]) - @unittest.expectedFailure # TODO: RUSTPYTHON; + (, ('',)) def test_reduce_mutating_builtins_iter(self): # This is a reproducer of issue #101765 # where iter `__reduce__` calls could lead to a segfault or SystemError diff --git a/crates/stdlib/src/array.rs b/crates/stdlib/src/array.rs index 3f33f23d7..8d5a0a095 100644 --- a/crates/stdlib/src/array.rs +++ b/crates/stdlib/src/array.rs @@ -19,7 +19,7 @@ mod array { builtins::{ PositionIterInternal, PyByteArray, PyBytes, PyBytesRef, PyDictRef, PyFloat, PyGenericAlias, PyInt, PyList, PyListRef, PyStr, PyStrRef, PyTupleRef, PyType, - PyTypeRef, + PyTypeRef, builtins_iter, }, class_or_notimplemented, convert::{ToPyObject, ToPyResult, TryFromBorrowedObject, TryFromObject}, @@ -1376,9 +1376,13 @@ mod array { #[pymethod] fn __reduce__(&self, vm: &VirtualMachine) -> PyTupleRef { - self.internal - .lock() - .builtins_iter_reduce(|x| x.clone().into(), vm) + let func = builtins_iter(vm); + self.internal.lock().reduce( + func, + |x| x.clone().into(), + |vm| vm.ctx.empty_tuple.clone().into(), + vm, + ) } } diff --git a/crates/vm/src/builtins/bytearray.rs b/crates/vm/src/builtins/bytearray.rs index 5a3aee590..91369624f 100644 --- a/crates/vm/src/builtins/bytearray.rs +++ b/crates/vm/src/builtins/bytearray.rs @@ -1,7 +1,7 @@ //! Implementation of the python bytearray object. use super::{ PositionIterInternal, PyBytes, PyBytesRef, PyDictRef, PyGenericAlias, PyIntRef, PyStrRef, - PyTuple, PyTupleRef, PyType, PyTypeRef, + PyTuple, PyTupleRef, PyType, PyTypeRef, iter::builtins_iter, }; use crate::{ AsObject, Context, Py, PyObject, PyObjectRef, PyPayload, PyRef, PyResult, TryFromObject, @@ -857,9 +857,13 @@ impl PyByteArrayIterator { } #[pymethod] fn __reduce__(&self, vm: &VirtualMachine) -> PyTupleRef { - self.internal - .lock() - .builtins_iter_reduce(|x| x.clone().into(), vm) + let func = builtins_iter(vm); + self.internal.lock().reduce( + func, + |x| x.clone().into(), + |vm| vm.ctx.empty_tuple.clone().into(), + vm, + ) } #[pymethod] diff --git a/crates/vm/src/builtins/bytes.rs b/crates/vm/src/builtins/bytes.rs index 64e00cdc9..1abe8a302 100644 --- a/crates/vm/src/builtins/bytes.rs +++ b/crates/vm/src/builtins/bytes.rs @@ -1,6 +1,6 @@ use super::{ PositionIterInternal, PyDictRef, PyGenericAlias, PyIntRef, PyStrRef, PyTuple, PyTupleRef, - PyType, PyTypeRef, + PyType, PyTypeRef, iter::builtins_iter, }; use crate::{ AsObject, Context, Py, PyObject, PyObjectRef, PyPayload, PyRef, PyResult, @@ -747,9 +747,13 @@ impl PyBytesIterator { #[pymethod] fn __reduce__(&self, vm: &VirtualMachine) -> PyTupleRef { - self.internal - .lock() - .builtins_iter_reduce(|x| x.clone().into(), vm) + let func = builtins_iter(vm); + self.internal.lock().reduce( + func, + |x| x.clone().into(), + |vm| vm.ctx.empty_tuple.clone().into(), + vm, + ) } #[pymethod] diff --git a/crates/vm/src/builtins/dict.rs b/crates/vm/src/builtins/dict.rs index fcb51c2ca..b70c5e6e4 100644 --- a/crates/vm/src/builtins/dict.rs +++ b/crates/vm/src/builtins/dict.rs @@ -898,7 +898,7 @@ macro_rules! dict_view { #[allow(clippy::redundant_closure_call)] #[pymethod] fn __reduce__(&self, vm: &VirtualMachine) -> PyTupleRef { - let iter = builtins_iter(vm).to_owned(); + let iter = builtins_iter(vm); let internal = self.internal.lock(); let entries = match &internal.status { IterStatus::Active(dict) => dict @@ -968,7 +968,7 @@ macro_rules! dict_view { #[allow(clippy::redundant_closure_call)] #[pymethod] fn __reduce__(&self, vm: &VirtualMachine) -> PyTupleRef { - let iter = builtins_reversed(vm).to_owned(); + let iter = builtins_reversed(vm); let internal = self.internal.lock(); // TODO: entries must be reversed too let entries = match &internal.status { diff --git a/crates/vm/src/builtins/enumerate.rs b/crates/vm/src/builtins/enumerate.rs index 29d51f420..3cd8f4e73 100644 --- a/crates/vm/src/builtins/enumerate.rs +++ b/crates/vm/src/builtins/enumerate.rs @@ -1,5 +1,6 @@ use super::{ IterStatus, PositionIterInternal, PyGenericAlias, PyIntRef, PyTupleRef, PyType, PyTypeRef, + iter::builtins_reversed, }; use crate::common::lock::{PyMutex, PyRwLock}; use crate::{ @@ -124,9 +125,13 @@ impl PyReverseSequenceIterator { #[pymethod] fn __reduce__(&self, vm: &VirtualMachine) -> PyTupleRef { - self.internal - .lock() - .builtins_reversed_reduce(|x| x.clone(), vm) + let func = builtins_reversed(vm); + self.internal.lock().reduce( + func, + |x| x.clone(), + |vm| vm.ctx.empty_tuple.clone().into(), + vm, + ) } } diff --git a/crates/vm/src/builtins/iter.rs b/crates/vm/src/builtins/iter.rs index 736303e95..b96fb6855 100644 --- a/crates/vm/src/builtins/iter.rs +++ b/crates/vm/src/builtins/iter.rs @@ -4,17 +4,14 @@ use super::{PyInt, PyTupleRef, PyType}; use crate::{ - Context, Py, PyObject, PyObjectRef, PyPayload, PyResult, VirtualMachine, + Context, Py, PyObjectRef, PyPayload, PyResult, VirtualMachine, class::PyClassImpl, function::ArgCallable, object::{Traverse, TraverseFn}, protocol::PyIterReturn, types::{IterNext, Iterable, SelfIter}, }; -use rustpython_common::{ - lock::{PyMutex, PyRwLock, PyRwLockUpgradableReadGuard}, - static_cell, -}; +use rustpython_common::lock::{PyMutex, PyRwLock, PyRwLockUpgradableReadGuard}; /// Marks status of iterator. #[derive(Debug, Clone)] @@ -71,33 +68,29 @@ impl PositionIterInternal { } } - fn _reduce(&self, func: PyObjectRef, f: F, vm: &VirtualMachine) -> PyTupleRef + /// Build a pickle-compatible reduce tuple. + /// + /// `func` must be resolved **before** acquiring any lock that guards this + /// `PositionIterInternal`, so that the builtins lookup cannot trigger + /// reentrant iterator access and deadlock. + pub fn reduce( + &self, + func: PyObjectRef, + active: F, + empty: E, + vm: &VirtualMachine, + ) -> PyTupleRef where F: FnOnce(&T) -> PyObjectRef, + E: FnOnce(&VirtualMachine) -> PyObjectRef, { if let IterStatus::Active(obj) = &self.status { - vm.new_tuple((func, (f(obj),), self.position)) + vm.new_tuple((func, (active(obj),), self.position)) } else { - vm.new_tuple((func, (vm.ctx.new_list(Vec::new()),))) + vm.new_tuple((func, (empty(vm),))) } } - pub fn builtins_iter_reduce(&self, f: F, vm: &VirtualMachine) -> PyTupleRef - where - F: FnOnce(&T) -> PyObjectRef, - { - let iter = builtins_iter(vm).to_owned(); - self._reduce(iter, f, vm) - } - - pub fn builtins_reversed_reduce(&self, f: F, vm: &VirtualMachine) -> PyTupleRef - where - F: FnOnce(&T) -> PyObjectRef, - { - let reversed = builtins_reversed(vm).to_owned(); - self._reduce(reversed, f, vm) - } - fn _next(&mut self, f: F, op: OP) -> PyResult where F: FnOnce(&T, usize) -> PyResult, @@ -160,18 +153,12 @@ impl PositionIterInternal { } } -pub fn builtins_iter(vm: &VirtualMachine) -> &PyObject { - static_cell! { - static INSTANCE: PyObjectRef; - } - INSTANCE.get_or_init(|| vm.builtins.get_attr("iter", vm).unwrap()) +pub fn builtins_iter(vm: &VirtualMachine) -> PyObjectRef { + vm.builtins.get_attr("iter", vm).unwrap() } -pub fn builtins_reversed(vm: &VirtualMachine) -> &PyObject { - static_cell! { - static INSTANCE: PyObjectRef; - } - INSTANCE.get_or_init(|| vm.builtins.get_attr("reversed", vm).unwrap()) +pub fn builtins_reversed(vm: &VirtualMachine) -> PyObjectRef { + vm.builtins.get_attr("reversed", vm).unwrap() } #[pyclass(module = false, name = "iterator", traverse)] @@ -211,7 +198,13 @@ impl PySequenceIterator { #[pymethod] fn __reduce__(&self, vm: &VirtualMachine) -> PyTupleRef { - self.internal.lock().builtins_iter_reduce(|x| x.clone(), vm) + let func = builtins_iter(vm); + self.internal.lock().reduce( + func, + |x| x.clone(), + |vm| vm.ctx.empty_tuple.clone().into(), + vm, + ) } #[pymethod] @@ -252,24 +245,47 @@ impl PyCallableIterator { status: PyRwLock::new(IterStatus::Active(callable)), } } + + #[pymethod] + fn __reduce__(&self, vm: &VirtualMachine) -> PyTupleRef { + let func = builtins_iter(vm); + let status = self.status.read(); + if let IterStatus::Active(callable) = &*status { + let callable_obj: PyObjectRef = callable.clone().into(); + vm.new_tuple((func, (callable_obj, self.sentinel.clone()))) + } else { + vm.new_tuple((func, (vm.ctx.empty_tuple.clone(),))) + } + } } impl SelfIter for PyCallableIterator {} impl IterNext for PyCallableIterator { fn next(zelf: &Py, vm: &VirtualMachine) -> PyResult { - let status = zelf.status.upgradable_read(); - let next = if let IterStatus::Active(callable) = &*status { - let ret = callable.invoke((), vm)?; - if vm.bool_eq(&ret, &zelf.sentinel)? { - *PyRwLockUpgradableReadGuard::upgrade(status) = IterStatus::Exhausted; - PyIterReturn::StopIteration(None) - } else { - PyIterReturn::Return(ret) + // Clone the callable and release the lock before invoking, + // so that reentrant next() calls don't deadlock. + let callable = { + let status = zelf.status.read(); + match &*status { + IterStatus::Active(callable) => callable.clone(), + IterStatus::Exhausted => return Ok(PyIterReturn::StopIteration(None)), } - } else { - PyIterReturn::StopIteration(None) }; - Ok(next) + + let ret = callable.invoke((), vm)?; + + // Re-check: a reentrant call may have exhausted the iterator. + let status = zelf.status.upgradable_read(); + if !matches!(&*status, IterStatus::Active(_)) { + return Ok(PyIterReturn::StopIteration(None)); + } + + if vm.bool_eq(&ret, &zelf.sentinel)? { + *PyRwLockUpgradableReadGuard::upgrade(status) = IterStatus::Exhausted; + Ok(PyIterReturn::StopIteration(None)) + } else { + Ok(PyIterReturn::Return(ret)) + } } } diff --git a/crates/vm/src/builtins/list.rs b/crates/vm/src/builtins/list.rs index 9c961d292..c6b9869a0 100644 --- a/crates/vm/src/builtins/list.rs +++ b/crates/vm/src/builtins/list.rs @@ -1,4 +1,7 @@ -use super::{PositionIterInternal, PyGenericAlias, PyTupleRef, PyType, PyTypeRef}; +use super::{ + PositionIterInternal, PyGenericAlias, PyTupleRef, PyType, PyTypeRef, + iter::{builtins_iter, builtins_reversed}, +}; use crate::atomic_func; use crate::common::lock::{ PyMappedRwLockReadGuard, PyMutex, PyRwLock, PyRwLockReadGuard, PyRwLockWriteGuard, @@ -577,9 +580,13 @@ impl PyListIterator { #[pymethod] fn __reduce__(&self, vm: &VirtualMachine) -> PyTupleRef { - self.internal - .lock() - .builtins_iter_reduce(|x| x.clone().into(), vm) + let func = builtins_iter(vm); + self.internal.lock().reduce( + func, + |x| x.clone().into(), + |vm| vm.ctx.new_list(Vec::new()).into(), + vm, + ) } } @@ -622,9 +629,13 @@ impl PyListReverseIterator { #[pymethod] fn __reduce__(&self, vm: &VirtualMachine) -> PyTupleRef { - self.internal - .lock() - .builtins_reversed_reduce(|x| x.clone().into(), vm) + let func = builtins_reversed(vm); + self.internal.lock().reduce( + func, + |x| x.clone().into(), + |vm| vm.ctx.new_list(Vec::new()).into(), + vm, + ) } } diff --git a/crates/vm/src/builtins/memory.rs b/crates/vm/src/builtins/memory.rs index 6db260c80..f625dfcf0 100644 --- a/crates/vm/src/builtins/memory.rs +++ b/crates/vm/src/builtins/memory.rs @@ -1,6 +1,6 @@ use super::{ PositionIterInternal, PyBytes, PyBytesRef, PyGenericAlias, PyInt, PyListRef, PySlice, PyStr, - PyStrRef, PyTuple, PyTupleRef, PyType, PyTypeRef, + PyStrRef, PyTuple, PyTupleRef, PyType, PyTypeRef, iter::builtins_iter, }; use crate::{ AsObject, Context, Py, PyObject, PyObjectRef, PyPayload, PyRef, PyResult, @@ -1132,9 +1132,13 @@ impl PyPayload for PyMemoryViewIterator { impl PyMemoryViewIterator { #[pymethod] fn __reduce__(&self, vm: &VirtualMachine) -> PyTupleRef { - self.internal - .lock() - .builtins_iter_reduce(|x| x.clone().into(), vm) + let func = builtins_iter(vm); + self.internal.lock().reduce( + func, + |x| x.clone().into(), + |vm| vm.ctx.empty_tuple.clone().into(), + vm, + ) } } diff --git a/crates/vm/src/builtins/range.rs b/crates/vm/src/builtins/range.rs index 49f50ffba..c0cd070c2 100644 --- a/crates/vm/src/builtins/range.rs +++ b/crates/vm/src/builtins/range.rs @@ -670,7 +670,7 @@ fn range_iter_reduce( index: usize, vm: &VirtualMachine, ) -> PyResult { - let iter = builtins_iter(vm).to_owned(); + let iter = builtins_iter(vm); let stop = start.clone() + length * step.clone(); let range = PyRange { start: PyInt::from(start).into_ref(&vm.ctx), diff --git a/crates/vm/src/builtins/set.rs b/crates/vm/src/builtins/set.rs index c34738097..0504eba0e 100644 --- a/crates/vm/src/builtins/set.rs +++ b/crates/vm/src/builtins/set.rs @@ -1390,7 +1390,7 @@ impl PySetIterator { ) -> PyResult<(PyObjectRef, (PyObjectRef,))> { let internal = zelf.internal.lock(); Ok(( - builtins_iter(vm).to_owned(), + builtins_iter(vm), (vm.ctx .new_list(match &internal.status { IterStatus::Exhausted => vec![], diff --git a/crates/vm/src/builtins/str.rs b/crates/vm/src/builtins/str.rs index 1721f921f..754732fab 100644 --- a/crates/vm/src/builtins/str.rs +++ b/crates/vm/src/builtins/str.rs @@ -1,7 +1,10 @@ use super::{ PositionIterInternal, PyBytesRef, PyDict, PyTupleRef, PyType, PyTypeRef, int::{PyInt, PyIntRef}, - iter::IterStatus::{self, Exhausted}, + iter::{ + IterStatus::{self, Exhausted}, + builtins_iter, + }, }; use crate::{ AsObject, Context, Py, PyExact, PyObject, PyObjectRef, PyPayload, PyRef, PyRefExact, PyResult, @@ -304,10 +307,13 @@ impl PyStrIterator { #[pymethod] fn __reduce__(&self, vm: &VirtualMachine) -> PyTupleRef { - self.internal - .lock() - .0 - .builtins_iter_reduce(|x| x.clone().into(), vm) + let func = builtins_iter(vm); + self.internal.lock().0.reduce( + func, + |x| x.clone().into(), + |vm| vm.ctx.empty_str.to_owned().into(), + vm, + ) } } diff --git a/crates/vm/src/builtins/tuple.rs b/crates/vm/src/builtins/tuple.rs index 8e4f158d8..368603374 100644 --- a/crates/vm/src/builtins/tuple.rs +++ b/crates/vm/src/builtins/tuple.rs @@ -1,4 +1,6 @@ -use super::{PositionIterInternal, PyGenericAlias, PyStrRef, PyType, PyTypeRef}; +use super::{ + PositionIterInternal, PyGenericAlias, PyStrRef, PyType, PyTypeRef, iter::builtins_iter, +}; use crate::common::{ hash::{PyHash, PyUHash}, lock::PyMutex, @@ -559,9 +561,13 @@ impl PyTupleIterator { #[pymethod] fn __reduce__(&self, vm: &VirtualMachine) -> PyTupleRef { - self.internal - .lock() - .builtins_iter_reduce(|x| x.clone().into(), vm) + let func = builtins_iter(vm); + self.internal.lock().reduce( + func, + |x| x.clone().into(), + |vm| vm.ctx.empty_tuple.clone().into(), + vm, + ) } }