GeezerVison is showing github.com — gh-148442: eliminate race condition in list rich comparison by KowalskiThomas · Pull Request #148531 · python/cpython
Conversation
What is this PR?
This PR is a fix for #148442, where the list's rich comparison function could return an incorrect result due a race condition in non-trivial __eq__ implementations of list elements.
It also updates test_equal_operator_modifying_operand which would previously assert two different lists were equal; it now properly asserts they are different.
Sorry, something went wrong.
Uh oh!
There was an error while loading. Please reload this page.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Could you please add a test specific to this issue, which shows why the new behavior can be better than the old one?
For example, let list1 = [x, 1] and list2 = [0, 0], where x > 0. During comparison list1 > list2, list1[0] is replaced with 0. Currently, the result will be False, even if both old and new values of list1 are larger than list2. Add also a test with replacing list2[0].
Sorry, something went wrong.
Uh oh!
There was an error while loading. Please reload this page.
KowalskiThomas reacted with thumbs up emoji
|
@serhiy-storchaka I added two tests (and rebased on main since the branch was pretty out of date).
|
Sorry, something went wrong.
Uh oh!
There was an error while loading. Please reload this page.
|
I afraid this is too heavyweight. No need to use threads if we can trigger the issue with a single thread.
|
Sorry, something went wrong.
Uh oh!
There was an error while loading. Please reload this page.
|
Ah yes, that's a very good point...
I just removed those tests and added two new ones that are single-threaded and do about the same thing with some... peculiar comparison operators.
Also, currently the release note says...
Fix a race condition in ``list.__lt__``, ``list.__le__``, ``list.__gt__``,
and ``list.__ge__``. Previously, under concurrent list mutation, the items
being compared could be replaced with other objects before the final comparison,
causing the final comparison result to be incorrect.
... should I also update it to frame it better and make it clear that it's not necessarily concurrent mutation? Even though that would be the most "usual" case I believe.
|
Sorry, something went wrong.
Uh oh!
There was an error while loading. Please reload this page.
|
This PR is stale because it has been open for 90 days with no activity.
|
Sorry, something went wrong.
Uh oh!
There was an error while loading. Please reload this page.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sorry, something went wrong.
Uh oh!
There was an error while loading. Please reload this page.
Uh oh!
There was an error while loading. Please reload this page.
Uh oh!
There was an error while loading. Please reload this page.
Assignees
No one assigned
Labels
stale
Stale PR or inactive for long period of time.
Milestone
No milestone
Development
Successfully merging this pull request may close these issues.
Uh oh!
There was an error while loading. Please reload this page.
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.
|