Replace key if not identical to old key in dict #31685

malthe · 2022-03-04T15:13:59Z

This fixes an issue in dict and by extension in weakref.WeakKeyDictionary which arises when a key that is equal to an existing key is used to set a new value – where the key itself is not replaced.

To appreciate this situation, consider when the keys are type weakref.ref.

In this case, the lifetime of the key after replacing the value is now that of the initial key and not the one that's been set subsequently – since the key entry is still the initial object. If the first key falls to refcount zero, the key is removed from the weakref.WeakKeyDictionary – which is unexpected because we have set a newer key which might have refcount greater than zero.

In the general case where a value is replaced for the exact same key (object identity), there is no practical difference with this change – and when a new key is used to replace a value, the cost is only a write to memory.

Thanks to @potiuk and @ashb for helping finding this issue which was discovered during flaky test runs in Apache Airflow.

potiuk · 2022-03-04T15:16:03Z

Nice :)

There is a very probable WeakKeyDict bug in Python standard library (to be confirmed and investigated further) that manifests itself in a very rare failure of the test_stacktrace_on_failure_starts_with_task_execute_method This turned out to be related to an unexpected behaviour (and most likely a bug - to be confirmed) of WeakKeyDict when you have potentially two different objects with the same `equals` and `hash` values added to the same WeakKeyDict as keys. More info on similar report (but raised for a bit different reason) bug in Python can be found here: https://bugs.python.org/issue44140 We submitted a PR to fix the problem found python/cpython#31685

methane · 2022-03-05T01:44:49Z

Thank you for first pull request. But we need an issue on bugs.python.org unless the pull request don't change any behavior (e.g. fixing typo, code cleanup).

But we are planning to moving issue tracker from bugs.python.org to GitHub issues in this month.
https://discuss.python.org/t/github-issues-migration-is-coming-soon/13791

All issues are copied to the GitHub issues.
During the migration (3-5 days estimated), you can not use both of bugs.python.org and GitHub issues.
If you don't want to learn bugs.python.org only for few days, I recommend to wait the migration finished.

In the general case where a value is replaced for the exact same key (object identity), there is no practical difference with this change – and when a new key is used to replace a value, the cost is only a write to memory.

This change is not so easy as you think. In Python, True == 1, False == 0, 2.0 == 2. We guaranteed "first wins" semantics for very long time. So it is not an option to change the dict.

I recommend you to start discussion in:

Both discussion can be used during issue tracker migration.

When posting your idea on the discussion, please describe your use case more concretely.

ashb · 2022-03-07T10:16:23Z

@malthe Might be worth turning this in to a doc update to mention this "gotcha" for the next people?

potiuk · 2022-03-07T10:42:09Z

@malthe Might be worth turning this in to a doc update to mention this "gotcha" for the next people?

Yep. We already asked for it https://bugs.python.org/issue46925 and Python maintainers agreed it's a good thing to do and reopened the issue to be "doc-issue".

There is a very probable WeakKeyDict bug in Python standard library (to be confirmed and investigated further) that manifests itself in a very rare failure of the test_stacktrace_on_failure_starts_with_task_execute_method This turned out to be related to an unexpected behaviour (and most likely a bug - to be confirmed) of WeakKeyDict when you have potentially two different objects with the same `equals` and `hash` values added to the same WeakKeyDict as keys. More info on similar report (but raised for a bit different reason) bug in Python can be found here: https://bugs.python.org/issue44140 We submitted a PR to fix the problem found python/cpython#31685 (cherry picked from commit 1949f5d)

There is a very probable WeakKeyDict bug in Python standard library (to be confirmed and investigated further) that manifests itself in a very rare failure of the test_stacktrace_on_failure_starts_with_task_execute_method This turned out to be related to an unexpected behaviour (and most likely a bug - to be confirmed) of WeakKeyDict when you have potentially two different objects with the same `equals` and `hash` values added to the same WeakKeyDict as keys. More info on similar report (but raised for a bit different reason) bug in Python can be found here: https://bugs.python.org/issue44140 We submitted a PR to fix the problem found python/cpython#31685 (cherry picked from commit 1949f5d76b5842d56db91c868ae4655bb7a7689f) GitOrigin-RevId: 91b95ea6633bc14ca9bdefe5eac0ac948465dfc9

There is a very probable WeakKeyDict bug in Python standard library (to be confirmed and investigated further) that manifests itself in a very rare failure of the test_stacktrace_on_failure_starts_with_task_execute_method This turned out to be related to an unexpected behaviour (and most likely a bug - to be confirmed) of WeakKeyDict when you have potentially two different objects with the same `equals` and `hash` values added to the same WeakKeyDict as keys. More info on similar report (but raised for a bit different reason) bug in Python can be found here: https://bugs.python.org/issue44140 We submitted a PR to fix the problem found python/cpython#31685 GitOrigin-RevId: 1949f5d76b5842d56db91c868ae4655bb7a7689f

bedevere-bot added the awaiting review label Mar 4, 2022

the-knights-who-say-ni added the CLA signed label Mar 4, 2022

malthe force-pushed the fix-weakref-key-replacement branch from f572737 to 4f8c6b6 Compare March 4, 2022 23:10

malthe requested review from methane and markshannon as code owners March 4, 2022 23:10

malthe changed the title ~~Keys should always be replaced in WeakKeyDictionary~~ Replace key if not identical to old key in dict Mar 4, 2022

Replace key if not identical to old key

0fd074a

malthe force-pushed the fix-weakref-key-replacement branch from 4f8c6b6 to 0fd074a Compare March 4, 2022 23:20

rhettinger closed this Mar 5, 2022

malthe mannequin mentioned this pull request Apr 10, 2022

Document dict behavior when setting equal but not identical key #91081

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Replace key if not identical to old key in dict #31685

Replace key if not identical to old key in dict #31685

malthe commented Mar 4, 2022 •

edited

Loading

potiuk commented Mar 4, 2022

methane commented Mar 5, 2022

ashb commented Mar 7, 2022

potiuk commented Mar 7, 2022

Replace key if not identical to old key in dict #31685

Replace key if not identical to old key in dict #31685

Conversation

malthe commented Mar 4, 2022 • edited Loading

potiuk commented Mar 4, 2022

methane commented Mar 5, 2022

ashb commented Mar 7, 2022

potiuk commented Mar 7, 2022

malthe commented Mar 4, 2022 •

edited

Loading