gh-148259: make list.remove atomic for non-trivial __eq__ comparison#148440
gh-148259: make list.remove atomic for non-trivial __eq__ comparison#148440KowalskiThomas wants to merge 3 commits intopython:mainfrom
list.remove atomic for non-trivial __eq__ comparison#148440Conversation
colesbury
left a comment
There was a problem hiding this comment.
Please add a NEWS entry (https://devguide.python.org/getting-started/pull-request-lifecycle/#how-to-add-a-news-entry) and fix the failing tests.
858e505 to
c30ea14
Compare
picnixz
left a comment
There was a problem hiding this comment.
The raises parameter is now misleading. Please correct it. In addition, please add tests to list itself. And please also update the docs if they need to.
Overall, I'm not convinced by this change. Mutating the list while doing the comparison is unsafe. We just don't want to crash. But making it raise a RuntimeError does not necessarily makes it better. Otherwise, we need to do it for all other mutable sequences in C to have more or less the same behavior.
cc @rhettinger
|
A Python core developer has requested some changes be made to your pull request before we can consider merging it. If you could please address their requests along with any other requests in other reviews from core developers that would be appreciated. Once you have made the requested changes, please leave a comment on this pull request containing the phrase |
What is this PR?
This PR addresses the bug reported in #148259 where calling rich comparison may result in releasing the GIL which may result in
list.removeremoving the wrong item from thelist.The PR also updates the tests added for #126033.
list.removeis not atomic for non trivial__eq__comparisons #148259