Fix PyDict issues on free-threaded build #4788
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Icxolu
reviewed
Dec 10, 2024
src/types/dict.rs
Outdated
| @@ -470,15 +478,15 @@ impl DictIterImpl { | |||
| let mut key: *mut ffi::PyObject = std::ptr::null_mut(); | |||
| let mut value: *mut ffi::PyObject = std::ptr::null_mut(); | |||
|
|
|||
| if unsafe { ffi::PyDict_Next(dict.as_ptr(), ppos, &mut key, &mut value) } != 0 { | |||
| if ffi::PyDict_Next(dict.as_ptr(), ppos, &mut key, &mut value) != 0 { | |||
There was a problem hiding this comment.
IMO we should keep these unsafe blocks. unsafe_op_in_unsafe_fn will be warn be default in the 2024 edition and it makes it easier validate and document the safety invariants.
There was a problem hiding this comment.
Thanks! For some reason I thought there was a clippy lint to remove these.
There was a problem hiding this comment.
It looks like Rust 1.63 objects:
https://github.com/PyO3/pyo3/actions/runs/12571738722/job/35043025705#step:11:107
Is there a way to keep this with the "unnecessary" unsafes and still compile on older rust?
There was a problem hiding this comment.
We could add a #[deny(unsafe_op_in_unsafe_fn)] on the function. Not the nicest solution I guess but it should work.
ngoldbaum
force-pushed
the
pydict-locking
branch
from
b6d1d36 to
9b42a6d
Compare
alex
approved these changes
Dec 24, 2024
github-merge-queue
bot
removed this pull request from the merge queue due to failed status checks
github-merge-queue
bot
removed this pull request from the merge queue due to failed status checks
davidhewitt
added a commit
that referenced
this pull request
Jan 3, 2025
* clarify safety docs for PyDict API * get dict size using PyDict_Size on free-threaded build * avoid unnecessary critical sections in PyDict * add changelog entry * fix build error on GIL-enabled build * address code review comments * move attribute below doc comment * ignore unsafe_op_in_unsafe_fn in next_unchecked --------- Co-authored-by: David Hewitt <[email protected]>
davidhewitt
added a commit
that referenced
this pull request
Jan 3, 2025
* clarify safety docs for PyDict API * get dict size using PyDict_Size on free-threaded build * avoid unnecessary critical sections in PyDict * add changelog entry * fix build error on GIL-enabled build * address code review comments * move attribute below doc comment * ignore unsafe_op_in_unsafe_fn in next_unchecked --------- Co-authored-by: David Hewitt <[email protected]>
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
This fixes two issues with PyDict on the free-threaded build.
We were acquiring an inner critical section in
next()in functions where we had already acquired an outer critical section. That is unnecessary. I've renamedDictIterImpl::DictIter::nexttonext_unchecked, marked itunsafe, and systematically used it in spots where we already had a critical section acquired. In the only spot we didn't, I manually added a critical section. I think having an unsafenext_uncheckedis easier to reason about - you need to do the locking yourself.We were accessing dict internals in a thread-unsafe manner, we should go through the C API on the free-threaded build. CPython uses a relaxed atomic load internally.
Also includes some minor doc improvements.
This should probably be backported to the 0.23 branch.
ping @bschoenmaeckers