Protect iterators against concurrent modification #2380
Conversation
src/types/dict.rs
Outdated
| /// # Panics | ||
| /// | ||
| /// If PyO3 detects that the dictionary is mutated during iteration, it will panic. | ||
| /// It is allowed to modify the values of the keys as you iterate over the dictionary, but only |
There was a problem hiding this comment.
I know "modify the values of the keys" is copied verbatim from the C-API docs, but I think it's quite confusing.
There was a problem hiding this comment.
I love this! 🚀
Does PyList need to be protected? It seems fine as is.
Good question. I guess it's by luck that with the current implementation we re-fetch the len() on each call to next()? I guess checking for modifications would require that and an extra check, so maybe it's ok as-is.
src/types/dict.rs
Outdated
| di_dict: *mut ffi::PyDictObject, | ||
| di_pos: ffi::Py_ssize_t, | ||
| di_used: ffi::Py_ssize_t, | ||
| marker: PhantomData<&'py PyDict>, |
There was a problem hiding this comment.
Is there a risk that the dict refcount can fall to 0 and can be accidentally released? Should this be Py<PyDict>?
There was a problem hiding this comment.
The iterator borrows from the Pydict, so this should be fine.
src/types/dict.rs
Outdated
|
|
||
| fn into_iter(self) -> Self::IntoIter { | ||
| unsafe { | ||
| let di_dict: *mut ffi::PyDictObject = self.as_ptr().cast(); |
There was a problem hiding this comment.
I've always wondered if there should be an as_raw() -> *mut ffi::PyDictObject method on PyDict (and similar for other types)? Available only on not (limited_api / PyPy).
There was a problem hiding this comment.
Sounds interesting, but I'm not sure how often this is useful in practice. You only need it to access fields directly, which we don't tend to do.
src/types/dict.rs
Outdated
| #[inline] | ||
| fn next(&mut self) -> Option<Self::Item> { | ||
| unsafe { | ||
| if self.di_used != (*(self.di_dict)).ma_used { |
There was a problem hiding this comment.
What's the difference between ma_used and PyDict_Size used below?
There was a problem hiding this comment.
Hmm...I read the cpython source some more: actually none. Let me reconsider the approach.
| impl PyFrozenSet { | ||
| /// Creates a new frozenset. | ||
| /// | ||
| /// May panic when running out of memory. |
There was a problem hiding this comment.
I know not from this PR, but is this actually true? It looks like returning PyResult means that we'll actually return PyErr containing a MemoryError if we run out of memory.
I suspect that actually the text may want to be more like PySet where we don't mention this and instead comment on the error return being caused by unhashable contents.
|
I know I already approved... Looks like this needs a CHANGELOG and then good to merge? |
|
Yes, I've been busy for a bit. I'll do it soon :) |
|
That should be all 🎉 |
| /// These iterators use Python's C-API directly. However in certain cases, like when compiling for | ||
| /// the Limited API and PyPy, the underlying structures are opaque and that may not be possible. | ||
| /// In these cases the iterators are implemented by forwarding to [`PyIterator`]. | ||
| pub mod iter { |
There was a problem hiding this comment.
👍 it was long overdue for these to be public!
Fixes #2162