I believe there is a race if __getitem__ sees the key exists in self.data and the GIL is released before getting the item. If another thread then calls __delitem__ which removes the item from self.data, when __getitem__ resumes it will attempt to get and return the item which no longer exists resulting in KeyError being raised out of __getitem__. If the class has __missing__ defined it should be called if the item is missing, but that won't occur in this sequence of events.
This is a theoretical bug report based solely on code inspection prompted by a DPO discussion about a different race the GIL does protect against (https://discuss.python.org/t/pep-805-safe-parallel-python/108670/49).
The only concern is that __missing__ will not be called, not that the item is initially seen then not seen.
I have not done a thorough review yet of the rest of the UserDict methods that do a containment check followed by lookup, but my cursory inspection they look to delegate to __getitem__. I am a bit concerned that get() does not call __missing__ if the initial check does not see the item but will if it sees the item and this bug is fixed as proposed below will result in __missing__ being called. This should be consistent.
#17910 seems related and I will look into the details of that fix to ensure this fix is consistent (if I end up working on this issue).
I offer a strawman proposal for fixing this by changing __getitem__ to use a single inspection of self.data with something like:
item = self.data.get(key, MISSING) if item is not MISSING: return item ...
Where MISSING is an internal object guaranteed to not be a key in self.data (adding it if something suitable doesn't already exist).
I volunteer to work on this issue. To point me in the right direction are there existing tests (either in collections or elsewhere) that force GIL timing races I can look at as an example of how to write a test to verify this issue is fixed? The challenge is reliably interleaving the initial containment check, then __delitem__, then the subscript access. I'm not sure if this level of testing is feasible based on previous experience with similar issues...the fix usually invalidates the test steps to interleave things properly by removing the ability for them to be interleaved. Any guidance on this would be very appreciated if my offer to work on this is taken up.
cpython/Lib/collections/__init__.py
Line 1189 in 71cb9e8
I believe there is a race if
__getitem__sees the key exists in self.data and the GIL is released before getting the item. If another thread then calls__delitem__which removes the item from self.data, when__getitem__resumes it will attempt to get and return the item which no longer exists resulting in KeyError being raised out of__getitem__. If the class has__missing__defined it should be called if the item is missing, but that won't occur in this sequence of events.This is a theoretical bug report based solely on code inspection prompted by a DPO discussion about a different race the GIL does protect against (https://discuss.python.org/t/pep-805-safe-parallel-python/108670/49).
The only concern is that
__missing__will not be called, not that the item is initially seen then not seen.I have not done a thorough review yet of the rest of the UserDict methods that do a containment check followed by lookup, but my cursory inspection they look to delegate to
__getitem__. I am a bit concerned that get() does not call__missing__if the initial check does not see the item but will if it sees the item and this bug is fixed as proposed below will result in__missing__being called. This should be consistent.#17910 seems related and I will look into the details of that fix to ensure this fix is consistent (if I end up working on this issue).
I offer a strawman proposal for fixing this by changing
__getitem__to use a single inspection of self.data with something like:item = self.data.get(key, MISSING) if item is not MISSING: return item ...Where MISSING is an internal object guaranteed to not be a key in self.data (adding it if something suitable doesn't already exist).
I volunteer to work on this issue. To point me in the right direction are there existing tests (either in collections or elsewhere) that force GIL timing races I can look at as an example of how to write a test to verify this issue is fixed? The challenge is reliably interleaving the initial containment check, then
__delitem__, then the subscript access. I'm not sure if this level of testing is feasible based on previous experience with similar issues...the fix usually invalidates the test steps to interleave things properly by removing the ability for them to be interleaved. Any guidance on this would be very appreciated if my offer to work on this is taken up.