gh-156544: fix free-threading race in UserDict with __missing__ or default - #157287
Open
gdchinacat wants to merge 3 commits into
Open
gh-156544: fix free-threading race in UserDict with __missing__ or default #157287gdchinacat wants to merge 3 commits into
gdchinacat wants to merge 3 commits into
Conversation
|
All commit authors signed the Contributor License Agreement. |
|
Most changes to Python require a NEWS entry. Add one using the blurb_it web app or the blurb command-line tool. If this change has little impact on Python users, wait for a maintainer to apply the |
…ing change to fix race condition. 1) ensure UserDict item access matches dict 2) test exactly how UserDict.get() and __getitem() interact with its wrapped dict. This commit does not make any functional changes, rather it documents in the form of tests what the existing interaction between UserDict and the dict it wraps is so that a subsequent commit that changes it will clearly show how it this interaction changes. The concern is the changes to make the fix may be considered breaking. Including the changes in behavior in the PR for the fix will make it easier for reviewers to see exactly what effect the change has.
…tem__(). When UserDict.__delitem__, pop(), __popitem__(), or clear() are called concurrently with get() or __getitem__ a race can occur in that get() or __getitem__ can see the item as being contained and then they use self.data[key] to return the item. However, if the item is removed after the containment check this subscript access will raise KeyError and get() and __getitem__() will propogate it, resulting in get() not returning default and __getitem() incorrectly skipping the call it should make to __missing__ (if defined). This fix detects this race condition by ignoring the KeyError after containment checks to allow default to be returned or __missing__ to be called. It is implemented in this unusual way that preserves the containment check rather than relying solely on KeyError in order to preserve existing semantics and performance characteristics. Removing the call to __contains__ on self.data would change how UserData interacts with it and could potentially break code that relies on this call. In addition to preserving the current semantics, get() with a sentinel is not used to atomically detect if the item is missing. Changing 'in' and '[key]' to a single call to get() would replace these optimized operators with a more expensive function call. Because exceptions incur overhead only when raised, this implementation preserves existing performance for all cases except when the race actually occurs, but ensures correct functionality when it does. It also preserves the interaction semantics with the wrapped dict.
gdchinacat
force-pushed
the
gh_156544_user_dict_missing_race
branch
from
September 10, 2026 21:33
965f037 to
21ed00f
Compare
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
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
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 race conditions with
UserDict.__getitem__()and.get(). When the key is removed concurrently__missing__may not be called or the default not returned, instead KeyError is raised.This occurs because the initial containment check can see the item as existing, it is then concurrently removed, and the subsequent subscript lookup raises KeyError. I have reproduced these races on free-threading builds but not GIL-enabled builds. @markshannon says this is because the bytecodes emitted by the compiler are such that the GIL will not be released during the execution of
__getitem__orget.I considered several alternatives to resolving this and settled on catching the KeyError when the race occurs because it preserves existing semantics with respect to the wrapped dict. I do not believe it introduces performance degradation because the added try/except only incurs material costs when the exception occurs which is the case this fix addresses. I considered using
self.data.get(key, MISSING)as well as removing the check an relying solely on KeyError from the subscripting, but both of these approaches that alter the semantics.This PR includes two changesets. The first adds tests to ensure the fix does not change the semantics of how UserDict interacts with the dict it wraps. I have it as a separate commit to show that the fix to UserDict does not alter the semantics. I can squash them into a single commit if that is preferable, please let me know if this is preferable.