gh-122208: Don't delivery PyDict_EVENT_ADDED until it can't fail #122207
Conversation
This file contains hidden or 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
DinoV
changed the title
DinoV
added
needs backport to 3.12
carljm
approved these changes
Jul 24, 2024
| @@ -1745,16 +1750,11 @@ insertdict(PyInterpreterState *interp, PyDictObject *mp, | |||
| MAINTAIN_TRACKING(mp, key, value); | |||
|
|
|||
| if (ix == DKIX_EMPTY) { | |||
| assert(!_PyDict_HasSplitTable(mp)); | |||
There was a problem hiding this comment.
Was it intentional to remove this assertion? Doesn't seem related to the dict watcher stuff.
There was a problem hiding this comment.
No, thanks for catching that, I'll remove it.
Luckily there was no debugging involved! Just trying to re-factor some existing code to use dict watchers and I was worried about what would happen when the updates would fail (as the existing code handled it). So this will actually make that somewhat cleaner!
|
Thanks @DinoV for the PR 🌮🎉.. I'm working now to backport this PR to: 3.12, 3.13. |
facebook-github-bot
pushed a commit
to facebookincubator/cinder
that referenced
this pull request
Jul 24, 2024
Summary: Back port of: python/cpython#122207 fixing python/cpython#122208 The current dictionary watchers implementation delivers the added event before it checks to see if we need to re-size the dictionary. This resize can fail so the value isn't added, and then the tracker is out of sync with the true state of the dictionary. This moves the delivery of the event to after any necessary allocations have happened. Reviewed By: jbower-fb Differential Revision: D60182094 fbshipit-source-id: f34940e98ce1caadeee364f9d126d35839661961
facebook-github-bot
pushed a commit
to facebookincubator/cinder
that referenced
this pull request
Jul 24, 2024
Summary: Back port of: python/cpython#122207 fixing python/cpython#122208 The current dictionary watchers implementation delivers the added event before it checks to see if we need to re-size the dictionary. This resize can fail so the value isn't added, and then the tracker is out of sync with the true state of the dictionary. This moves the delivery of the event to after any necessary allocations have happened. Reviewed By: jbower-fb Differential Revision: D60182094 fbshipit-source-id: f34940e98ce1caadeee364f9d126d35839661961
nohlson
pushed a commit
to nohlson/cpython
that referenced
this pull request
Jul 24, 2024
python#122207) Don't delivery PyDict_EVENT_ADDED until it can't fail
nohlson
pushed a commit
to nohlson/cpython
that referenced
this pull request
Jul 24, 2024
python#122207) Don't delivery PyDict_EVENT_ADDED until it can't fail
hugovk
removed
needs backport to 3.12
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.
Currently
PyDict_EVENT_ADDEDcan be delivered when the insert is going to fail due to an OOM. This means the listener has no capability of knowing the true state of the dictionary if it fails.This modifies the events to be delivered before the change has happened but only after any underlying necessary allocations have occurred.
addedevent before it's guaranteed to be successful leading to possible incosistent state #122208