bpo-44962 fix a race in WeakKeyDict, WeakValueDict and WeakSet when two threads attempt to commit the last pending removal #27921

graingert · 2021-08-23T19:54:40Z

https://bugs.python.org/issue44962

Lib/_weakrefset.py

Misc/NEWS.d/next/Core and Builtins/2021-08-23-19-55-08.bpo-44962.J00ftt.rst

…62.J00ftt.rst

bensimner

The change being overriden here was introduced by @pitrou in c1baa60#diff-0d950b54d0d2b4cc98bbe2945b67e8e5c12ec14636861705e275aa86b35d73cf

The above change also touches WeakKeyDictionary and WeakValueDictionary too, should those also get the updated _commit_removals change? are there any other races that should be fixed?

fixes: Exception ignored in: weakref callback <function WeakKeyDictionary.__init__.<locals>.remove at 0x00007fe82245d2e0> Traceback (most recent call last): File "/usr/lib/pypy3/lib-python/3/weakref.py", line 390, in remove del self.data[k] KeyError: <weakref at 0x00007fe76e8d8180; dead> Exception ignored in: weakref callback <function WeakKeyDictionary.__init__.<locals>.remove at 0x00007fe82245d2e0> Traceback (most recent call last): File "/usr/lib/pypy3/lib-python/3/weakref.py", line 390, in remove del self.data[k] KeyError: <weakref at 0x00007fe76e8d81a0; dead> Exception ignored in: weakref callback <function WeakKeyDictionary.__init__.<locals>.remove at 0x00007fe82245d2e0> Traceback (most recent call last): File "/usr/lib/pypy3/lib-python/3/weakref.py", line 390, in remove del self.data[k] KeyError: <weakref at 0x000056548f1e24a0; dead> see agronholm/anyio#362 (comment)

Misc/NEWS.d/next/Core and Builtins/2021-08-23-19-55-08.bpo-44962.J00ftt.rst

…62.J00ftt.rst

Misc/NEWS.d/next/Core and Builtins/2021-08-23-19-55-08.bpo-44962.J00ftt.rst

…62.J00ftt.rst

Lib/weakref.py

Co-authored-by: Łukasz Langa <lukasz@langa.pl>

Lib/weakref.py

ambv · 2021-08-28T10:22:03Z

Lib/weakref.py

            except KeyError:
                pass


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.

Lib/weakref.py

see also https://bugs.python.org/issue29519

Lib/weakref.py

ambv

Normally we request tests for bug fixes but in the case of race conditions, they tend to be flaky.

miss-islington · 2021-08-28T17:07:40Z

Thanks @graingert for the PR, and @ambv for merging it 🌮🎉.. I'm working now to backport this PR to: 3.9, 3.10.
🐍🍒⛏🤖

bedevere-bot · 2021-08-28T17:07:57Z

GH-28013 is a backport of this pull request to the 3.10 branch.

…two threads attempt to commit the last pending removal (pythonGH-27921) Fixes: Traceback (most recent call last): File "/home/graingert/projects/asyncio-demo/demo.py", line 36, in <module> sys.exit(main()) File "/home/graingert/projects/asyncio-demo/demo.py", line 30, in main test_all_tasks_threading() File "/home/graingert/projects/asyncio-demo/demo.py", line 24, in test_all_tasks_threading results.append(f.result()) File "/usr/lib/python3.10/concurrent/futures/_base.py", line 438, in result return self.__get_result() File "/usr/lib/python3.10/concurrent/futures/_base.py", line 390, in __get_result raise self._exception File "/usr/lib/python3.10/concurrent/futures/thread.py", line 52, in run result = self.fn(*self.args, **self.kwargs) File "/usr/lib/python3.10/asyncio/runners.py", line 47, in run _cancel_all_tasks(loop) File "/usr/lib/python3.10/asyncio/runners.py", line 56, in _cancel_all_tasks to_cancel = tasks.all_tasks(loop) File "/usr/lib/python3.10/asyncio/tasks.py", line 53, in all_tasks tasks = list(_all_tasks) File "/usr/lib/python3.10/_weakrefset.py", line 60, in __iter__ with _IterationGuard(self): File "/usr/lib/python3.10/_weakrefset.py", line 33, in __exit__ w._commit_removals() File "/usr/lib/python3.10/_weakrefset.py", line 57, in _commit_removals discard(l.pop()) IndexError: pop from empty list Also fixes: Exception ignored in: weakref callback <function WeakKeyDictionary.__init__.<locals>.remove at 0x00007fe82245d2e0> Traceback (most recent call last): File "/usr/lib/pypy3/lib-python/3/weakref.py", line 390, in remove del self.data[k] KeyError: <weakref at 0x00007fe76e8d8180; dead> Exception ignored in: weakref callback <function WeakKeyDictionary.__init__.<locals>.remove at 0x00007fe82245d2e0> Traceback (most recent call last): File "/usr/lib/pypy3/lib-python/3/weakref.py", line 390, in remove del self.data[k] KeyError: <weakref at 0x00007fe76e8d81a0; dead> Exception ignored in: weakref callback <function WeakKeyDictionary.__init__.<locals>.remove at 0x00007fe82245d2e0> Traceback (most recent call last): File "/usr/lib/pypy3/lib-python/3/weakref.py", line 390, in remove del self.data[k] KeyError: <weakref at 0x000056548f1e24a0; dead> See: https://github.com/agronholm/anyio/issues/362GH-issuecomment-904424310 See also: https://bugs.python.org/issue29519 Co-authored-by: Łukasz Langa <lukasz@langa.pl> (cherry picked from commit 206b21e) Co-authored-by: Thomas Grainger <tagrain@gmail.com>

bedevere-bot · 2021-08-28T17:08:02Z

GH-28014 is a backport of this pull request to the 3.9 branch.

…two threads attempt to commit the last pending removal (pythonGH-27921) Fixes: Traceback (most recent call last): File "/home/graingert/projects/asyncio-demo/demo.py", line 36, in <module> sys.exit(main()) File "/home/graingert/projects/asyncio-demo/demo.py", line 30, in main test_all_tasks_threading() File "/home/graingert/projects/asyncio-demo/demo.py", line 24, in test_all_tasks_threading results.append(f.result()) File "/usr/lib/python3.10/concurrent/futures/_base.py", line 438, in result return self.__get_result() File "/usr/lib/python3.10/concurrent/futures/_base.py", line 390, in __get_result raise self._exception File "/usr/lib/python3.10/concurrent/futures/thread.py", line 52, in run result = self.fn(*self.args, **self.kwargs) File "/usr/lib/python3.10/asyncio/runners.py", line 47, in run _cancel_all_tasks(loop) File "/usr/lib/python3.10/asyncio/runners.py", line 56, in _cancel_all_tasks to_cancel = tasks.all_tasks(loop) File "/usr/lib/python3.10/asyncio/tasks.py", line 53, in all_tasks tasks = list(_all_tasks) File "/usr/lib/python3.10/_weakrefset.py", line 60, in __iter__ with _IterationGuard(self): File "/usr/lib/python3.10/_weakrefset.py", line 33, in __exit__ w._commit_removals() File "/usr/lib/python3.10/_weakrefset.py", line 57, in _commit_removals discard(l.pop()) IndexError: pop from empty list Also fixes: Exception ignored in: weakref callback <function WeakKeyDictionary.__init__.<locals>.remove at 0x00007fe82245d2e0> Traceback (most recent call last): File "/usr/lib/pypy3/lib-python/3/weakref.py", line 390, in remove del self.data[k] KeyError: <weakref at 0x00007fe76e8d8180; dead> Exception ignored in: weakref callback <function WeakKeyDictionary.__init__.<locals>.remove at 0x00007fe82245d2e0> Traceback (most recent call last): File "/usr/lib/pypy3/lib-python/3/weakref.py", line 390, in remove del self.data[k] KeyError: <weakref at 0x00007fe76e8d81a0; dead> Exception ignored in: weakref callback <function WeakKeyDictionary.__init__.<locals>.remove at 0x00007fe82245d2e0> Traceback (most recent call last): File "/usr/lib/pypy3/lib-python/3/weakref.py", line 390, in remove del self.data[k] KeyError: <weakref at 0x000056548f1e24a0; dead> See: https://github.com/agronholm/anyio/issues/362GH-issuecomment-904424310 See also: https://bugs.python.org/issue29519 Co-authored-by: Łukasz Langa <lukasz@langa.pl> (cherry picked from commit 206b21e) Co-authored-by: Thomas Grainger <tagrain@gmail.com>

…two threads attempt to commit the last pending removal (GH-27921) Fixes: Traceback (most recent call last): File "/home/graingert/projects/asyncio-demo/demo.py", line 36, in <module> sys.exit(main()) File "/home/graingert/projects/asyncio-demo/demo.py", line 30, in main test_all_tasks_threading() File "/home/graingert/projects/asyncio-demo/demo.py", line 24, in test_all_tasks_threading results.append(f.result()) File "/usr/lib/python3.10/concurrent/futures/_base.py", line 438, in result return self.__get_result() File "/usr/lib/python3.10/concurrent/futures/_base.py", line 390, in __get_result raise self._exception File "/usr/lib/python3.10/concurrent/futures/thread.py", line 52, in run result = self.fn(*self.args, **self.kwargs) File "/usr/lib/python3.10/asyncio/runners.py", line 47, in run _cancel_all_tasks(loop) File "/usr/lib/python3.10/asyncio/runners.py", line 56, in _cancel_all_tasks to_cancel = tasks.all_tasks(loop) File "/usr/lib/python3.10/asyncio/tasks.py", line 53, in all_tasks tasks = list(_all_tasks) File "/usr/lib/python3.10/_weakrefset.py", line 60, in __iter__ with _IterationGuard(self): File "/usr/lib/python3.10/_weakrefset.py", line 33, in __exit__ w._commit_removals() File "/usr/lib/python3.10/_weakrefset.py", line 57, in _commit_removals discard(l.pop()) IndexError: pop from empty list Also fixes: Exception ignored in: weakref callback <function WeakKeyDictionary.__init__.<locals>.remove at 0x00007fe82245d2e0> Traceback (most recent call last): File "/usr/lib/pypy3/lib-python/3/weakref.py", line 390, in remove del self.data[k] KeyError: <weakref at 0x00007fe76e8d8180; dead> Exception ignored in: weakref callback <function WeakKeyDictionary.__init__.<locals>.remove at 0x00007fe82245d2e0> Traceback (most recent call last): File "/usr/lib/pypy3/lib-python/3/weakref.py", line 390, in remove del self.data[k] KeyError: <weakref at 0x00007fe76e8d81a0; dead> Exception ignored in: weakref callback <function WeakKeyDictionary.__init__.<locals>.remove at 0x00007fe82245d2e0> Traceback (most recent call last): File "/usr/lib/pypy3/lib-python/3/weakref.py", line 390, in remove del self.data[k] KeyError: <weakref at 0x000056548f1e24a0; dead> See: https://github.com/agronholm/anyio/issues/362GH-issuecomment-904424310 See also: https://bugs.python.org/issue29519 Co-authored-by: Łukasz Langa <lukasz@langa.pl> (cherry picked from commit 206b21e) Co-authored-by: Thomas Grainger <tagrain@gmail.com>

…two threads attempt to commit the last pending removal (GH-27921) (GH-28014) Fixes: Traceback (most recent call last): File "/home/graingert/projects/asyncio-demo/demo.py", line 36, in <module> sys.exit(main()) File "/home/graingert/projects/asyncio-demo/demo.py", line 30, in main test_all_tasks_threading() File "/home/graingert/projects/asyncio-demo/demo.py", line 24, in test_all_tasks_threading results.append(f.result()) File "/usr/lib/python3.10/concurrent/futures/_base.py", line 438, in result return self.__get_result() File "/usr/lib/python3.10/concurrent/futures/_base.py", line 390, in __get_result raise self._exception File "/usr/lib/python3.10/concurrent/futures/thread.py", line 52, in run result = self.fn(*self.args, **self.kwargs) File "/usr/lib/python3.10/asyncio/runners.py", line 47, in run _cancel_all_tasks(loop) File "/usr/lib/python3.10/asyncio/runners.py", line 56, in _cancel_all_tasks to_cancel = tasks.all_tasks(loop) File "/usr/lib/python3.10/asyncio/tasks.py", line 53, in all_tasks tasks = list(_all_tasks) File "/usr/lib/python3.10/_weakrefset.py", line 60, in __iter__ with _IterationGuard(self): File "/usr/lib/python3.10/_weakrefset.py", line 33, in __exit__ w._commit_removals() File "/usr/lib/python3.10/_weakrefset.py", line 57, in _commit_removals discard(l.pop()) IndexError: pop from empty list Also fixes: Exception ignored in: weakref callback <function WeakKeyDictionary.__init__.<locals>.remove at 0x00007fe82245d2e0> Traceback (most recent call last): File "/usr/lib/pypy3/lib-python/3/weakref.py", line 390, in remove del self.data[k] KeyError: <weakref at 0x00007fe76e8d8180; dead> Exception ignored in: weakref callback <function WeakKeyDictionary.__init__.<locals>.remove at 0x00007fe82245d2e0> Traceback (most recent call last): File "/usr/lib/pypy3/lib-python/3/weakref.py", line 390, in remove del self.data[k] KeyError: <weakref at 0x00007fe76e8d81a0; dead> Exception ignored in: weakref callback <function WeakKeyDictionary.__init__.<locals>.remove at 0x00007fe82245d2e0> Traceback (most recent call last): File "/usr/lib/pypy3/lib-python/3/weakref.py", line 390, in remove del self.data[k] KeyError: <weakref at 0x000056548f1e24a0; dead> See: https://github.com/agronholm/anyio/issues/362GH-issuecomment-904424310 See also: https://bugs.python.org/issue29519 Co-authored-by: Łukasz Langa <lukasz@langa.pl> (cherry picked from commit 206b21e) Co-authored-by: Thomas Grainger <tagrain@gmail.com>

bpo-44962 fix a race in WeakSet

5a0dc76

the-knights-who-say-ni added the CLA signed label Aug 23, 2021

bedevere-bot added the awaiting review label Aug 23, 2021

📜🤖 Added by blurb_it.

3f74f24

graingert marked this pull request as ready for review August 23, 2021 19:56

graingert mentioned this pull request Aug 23, 2021

bpo-44962: protect asyncio.tasks._all_tasks WeakSet with a lock #27909

Closed

graingert commented Aug 24, 2021

View reviewed changes

Lib/_weakrefset.py Show resolved Hide resolved

graingert changed the title ~~bpo-44962 fix a race in WeakSet~~ bpo-44962 fix a race in WeakSet when two threads attempt to commit the last pending removal Aug 24, 2021

graingert commented Aug 24, 2021

View reviewed changes

Misc/NEWS.d/next/Core and Builtins/2021-08-23-19-55-08.bpo-44962.J00ftt.rst Outdated Show resolved Hide resolved

Update Misc/NEWS.d/next/Core and Builtins/2021-08-23-19-55-08.bpo-449…

08f31cb

…62.J00ftt.rst

bensimner reviewed Aug 24, 2021

View reviewed changes

graingert added 2 commits August 24, 2021 10:48

fix races in WeakKeyDict and WeakValueDict

738dc9a

graingert changed the title ~~bpo-44962 fix a race in WeakSet when two threads attempt to commit the last pending removal~~ bpo-44962 fix a race in WeakKeyDict, WeakValueDict and WeakSet when two threads attempt to commit the last pending removal Aug 24, 2021

graingert commented Aug 24, 2021

View reviewed changes

Misc/NEWS.d/next/Core and Builtins/2021-08-23-19-55-08.bpo-44962.J00ftt.rst Outdated Show resolved Hide resolved

Update Misc/NEWS.d/next/Core and Builtins/2021-08-23-19-55-08.bpo-449…

6476722

…62.J00ftt.rst

graingert commented Aug 24, 2021

View reviewed changes

Misc/NEWS.d/next/Core and Builtins/2021-08-23-19-55-08.bpo-44962.J00ftt.rst Outdated Show resolved Hide resolved

Update Misc/NEWS.d/next/Core and Builtins/2021-08-23-19-55-08.bpo-449…

36fd3f0

…62.J00ftt.rst

ambv requested a review from pitrou August 24, 2021 16:40

ambv reviewed Aug 28, 2021

View reviewed changes

Lib/weakref.py Outdated Show resolved Hide resolved

Update Lib/weakref.py

54a0032

Co-authored-by: Łukasz Langa <lukasz@langa.pl>

ambv reviewed Aug 28, 2021

View reviewed changes

Lib/weakref.py Outdated Show resolved Hide resolved

ambv reviewed Aug 28, 2021

View reviewed changes

ambv added needs backport to 3.9 needs backport to 3.10 only security fixes labels Aug 28, 2021

ambv reviewed Aug 28, 2021

View reviewed changes

Lib/weakref.py Show resolved Hide resolved

Lib/weakref.py Outdated Show resolved Hide resolved

graingert added 2 commits August 28, 2021 12:55

cache _atomic_removal in WeakValueDictionary._commit_removals

aada0e4

see also https://bugs.python.org/issue29519

remove redundant else clauses

b2b25d0

graingert commented Aug 28, 2021

View reviewed changes

Lib/weakref.py Outdated Show resolved Hide resolved

graingert requested a review from ambv August 28, 2021 12:05

ambv approved these changes Aug 28, 2021

View reviewed changes

bedevere-bot added awaiting merge and removed awaiting review labels Aug 28, 2021

ambv merged commit 206b21e into python:main Aug 28, 2021

bedevere-bot removed the awaiting merge label Aug 28, 2021

bedevere-bot removed the needs backport to 3.10 only security fixes label Aug 28, 2021

bedevere-bot removed the needs backport to 3.9 label Aug 28, 2021

graingert deleted the weakset-threadsafe branch August 28, 2021 18:53

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

Uh oh!

bpo-44962 fix a race in WeakKeyDict, WeakValueDict and WeakSet when two threads attempt to commit the last pending removal #27921

bpo-44962 fix a race in WeakKeyDict, WeakValueDict and WeakSet when two threads attempt to commit the last pending removal #27921

Uh oh!

graingert commented Aug 23, 2021 •

edited by bedevere-bot

Loading

Uh oh!

Uh oh!

Uh oh!

bensimner left a comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

ambv Aug 28, 2021

Uh oh!

Uh oh!

Uh oh!

Uh oh!

ambv left a comment

Uh oh!

miss-islington commented Aug 28, 2021

Uh oh!

bedevere-bot commented Aug 28, 2021

Uh oh!

bedevere-bot commented Aug 28, 2021

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

6 participants

Uh oh!

bpo-44962 fix a race in WeakKeyDict, WeakValueDict and WeakSet when two threads attempt to commit the last pending removal #27921

bpo-44962 fix a race in WeakKeyDict, WeakValueDict and WeakSet when two threads attempt to commit the last pending removal #27921

Uh oh!

Conversation

graingert commented Aug 23, 2021 • edited by bedevere-bot Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Uh oh!

Uh oh!

bensimner left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

ambv Aug 28, 2021

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

ambv left a comment

Choose a reason for hiding this comment

Uh oh!

miss-islington commented Aug 28, 2021

Uh oh!

bedevere-bot commented Aug 28, 2021

Uh oh!

bedevere-bot commented Aug 28, 2021

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

6 participants

graingert commented Aug 23, 2021 •

edited by bedevere-bot

Loading