Skip to content
Merged
10 changes: 7 additions & 3 deletions Lib/_weakrefset.py
Original file line number Diff line number Diff line change
Expand Up @@ -51,10 +51,14 @@ def _remove(item, selfref=ref(self)):
self.update(data)

def _commit_removals(self):
l = self._pending_removals
pop = self._pending_removals.pop
discard = self.data.discard
while l:
discard(l.pop())
while True:
try:
item = pop()
except IndexError:
return
discard(item)

def __iter__(self):
with _IterationGuard(self):
Expand Down
29 changes: 20 additions & 9 deletions Lib/weakref.py
Original file line number Diff line number Diff line change
Expand Up @@ -119,14 +119,17 @@ def remove(wr, selfref=ref(self), _atomic_removal=_remove_dead_weakref):
self.data = {}
self.update(other, **kw)

def _commit_removals(self):
l = self._pending_removals
def _commit_removals(self, _atomic_removal=_remove_dead_weakref):
pop = self._pending_removals.pop
d = self.data
# We shouldn't encounter any KeyError, because this method should
# always be called *before* mutating the dict.
while l:
key = l.pop()
_remove_dead_weakref(d, key)
while True:
try:
key = pop()
except IndexError:
return
_atomic_removal(d, key)

def __getitem__(self, key):
if self._pending_removals:
Expand Down Expand Up @@ -370,7 +373,10 @@ def remove(k, selfref=ref(self)):
if self._iterating:
self._pending_removals.append(k)
else:
del self.data[k]
try:
del self.data[k]
except KeyError:
pass
self._remove = remove
# A list of dead weakrefs (keys to be removed)
self._pending_removals = []
Expand All @@ -384,11 +390,16 @@ def _commit_removals(self):
# because a dead weakref never compares equal to a live weakref,
# even if they happened to refer to equal objects.
# However, it means keys may already have been removed.
l = self._pending_removals
pop = self._pending_removals.pop
d = self.data
while l:
while True:
try:
key = pop()
except IndexError:
return

try:
del d[l.pop()]
del d[key]
except KeyError:
pass
Comment on lines 403 to 404
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Observation: it kind of weirded me out there has to be a KeyError guard here. Even in the old code once you get something popped out of the list, you should be able to address the data key with it, no? There should be no duplicates in self._pending_removals!

Well, after some digging I found this isn't about duplicates. This is about a race where one remove() call hits the object while it's still _iterating (registering the key in _pending_removals), and concurrently another remove() call coming just a bit later when _iterating isn't set anymore (performing an eager removal).

In fact, while _remove_dead_weakref is called without a try:except: fence, it internally ignores raised KeyErrors. Similarly, the WeakSet is using discard.

There's some relevant info in https://bugs.python.org/issue21173 and https://bugs.python.org/issue28427.


Expand Down
Original file line number Diff line number Diff line change
@@ -0,0 +1 @@
Fix a race in WeakKeyDictionary, WeakValueDictionary and WeakSet when two threads attempt to commit the last pending removal. This fixes asyncio.create_task and fixes a data loss in asyncio.run where shutdown_asyncgens is not run