| FazBrowse GitHub Viewer | Trending | | Home |
| Tools: [Download Repo ZIP] [Original HTTPS Page] |
Sorry, something went wrong.
Sorry, something went wrong.
|
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 skip news label instead. |
Sorry, something went wrong.
…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.
| Back | FazBrowse Home | New Git URL |
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__ or get.
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.