The Wayback Machine - https://web.archive.org/web/20210905203504/https://github.com/python/cpython/pull/27921
Skip to content

/ cpython

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

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

Merged
merged 10 commits into from Aug 28, 2021
Merged

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

merged 10 commits into from Aug 28, 2021
+28 −12

Conversation

@graingert
Copy link
Contributor

@graingert graingert commented Aug 23, 2021 •
edited by bedevere-bot

https://bugs.python.org/issue44962
@graingert
bpo-44962 fix a race in WeakSet
5a0dc76
@blurb-it
📜🤖 Added by blurb_it.
3f74f24
@graingert graingert marked this pull request as ready for review Aug 23, 2021
@graingert graingert mentioned this pull request Aug 23, 2021
Closed
@graingert
graingert reviewed Aug 24, 2021
View changes
Lib/_weakrefset.py Show resolved Hide resolved
@graingert 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
graingert reviewed Aug 24, 2021
View changes
Misc/NEWS.d/next/Core and Builtins/2021-08-23-19-55-08.bpo-44962.J00ftt.rst Outdated Show resolved Hide resolved
@graingert
Update Misc/NEWS.d/next/Core and Builtins/2021-08-23-19-55-08.bpo-44962…
08f31cb 
….J00ftt.rst
@bensimner
bensimner reviewed Aug 24, 2021
View changes
Copy link

@bensimner bensimner left a comment

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?
graingert added 2 commits Aug 24, 2021
@graingert
fix races in WeakKeyDict and WeakValueDict
738dc9a
@graingert
catch KeyError in WeakKeyDict callback
f3585ef 
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)
@graingert 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
graingert reviewed Aug 24, 2021
View changes
Misc/NEWS.d/next/Core and Builtins/2021-08-23-19-55-08.bpo-44962.J00ftt.rst Outdated Show resolved Hide resolved
@graingert
Update Misc/NEWS.d/next/Core and Builtins/2021-08-23-19-55-08.bpo-44962…
6476722 
….J00ftt.rst
@graingert
graingert reviewed Aug 24, 2021
View changes
Misc/NEWS.d/next/Core and Builtins/2021-08-23-19-55-08.bpo-44962.J00ftt.rst Outdated Show resolved Hide resolved
@graingert
Update Misc/NEWS.d/next/Core and Builtins/2021-08-23-19-55-08.bpo-44962…
36fd3f0 
….J00ftt.rst
@ambv ambv requested a review from pitrou Aug 24, 2021
@ambv
ambv reviewed Aug 28, 2021
View changes
Lib/weakref.py Outdated Show resolved Hide resolved
@graingert @ambv
Update Lib/weakref.py
54a0032 
Co-authored-by: Łukasz Langa <[email protected]>
@ambv
ambv reviewed Aug 28, 2021
View changes
Lib/weakref.py Outdated Show resolved Hide resolved
@ambv
ambv reviewed Aug 28, 2021
View changes
Lib/weakref.py
except KeyError:
pass
Comment on lines 404 to 405

This comment has been minimized.

Sign in to view
@ambv

ambv Aug 28, 2021
Contributor

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.

@ambv
ambv reviewed Aug 28, 2021
View changes
Lib/weakref.py Show resolved Hide resolved
Lib/weakref.py Outdated Show resolved Hide resolved
graingert added 2 commits Aug 28, 2021
@graingert
cache _atomic_removal in WeakValueDictionary._commit_removals
aada0e4 
see also https://bugs.python.org/issue29519
@graingert
remove redundant else clauses
b2b25d0
@graingert
graingert reviewed Aug 28, 2021
View changes
Lib/weakref.py Outdated Show resolved Hide resolved
@graingert graingert requested a review from ambv Aug 28, 2021
@ambv
ambv approved these changes Aug 28, 2021
View changes
Copy link
Contributor

@ambv ambv left a comment

Normally we request tests for bug fixes but in the case of race conditions, they tend to be flaky.
@ambv ambv merged commit 206b21e into python:main Aug 28, 2021
12 checks passed
12 checks passed
@github-actions
Docs
Details
@github-actions
Check for source changes
Details
@github-actions
Check if generated files are up to date
Details
@github-actions
Windows (x86)
Details
@github-actions
Windows (x64)
Details
@github-actions
macOS
Details
@github-actions
Ubuntu
Details
@github-actions
Ubuntu SSL tests with OpenSSL
Details
@github-actions
Address sanitizer Address sanitizer
Details
Azure Pipelines PR #20210828.9 succeeded
Details
@bedevere-bot
bedevere/issue-number Issue number 44962 found
Details
@bedevere-bot
bedevere/news News entry found in Misc/NEWS.d
@miss-islington
Copy link
Contributor

@miss-islington miss-islington commented Aug 28, 2021

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
Copy link

@bedevere-bot bedevere-bot commented Aug 28, 2021

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

miss-islington added a commit to miss-islington/cpython that referenced this pull request Aug 28, 2021
@graingert @miss-islington
bpo-44962: Fix a race in WeakKeyDict, WeakValueDict and WeakSet when …
7ad41f0 
…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 <[email protected]>
(cherry picked from commit 206b21e)

Co-authored-by: Thomas Grainger <[email protected]>
@bedevere-bot
Copy link

@bedevere-bot bedevere-bot commented Aug 28, 2021

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

miss-islington added a commit to miss-islington/cpython that referenced this pull request Aug 28, 2021
@graingert @miss-islington
bpo-44962: Fix a race in WeakKeyDict, WeakValueDict and WeakSet when …
e978d6a 
…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 <[email protected]>
(cherry picked from commit 206b21e)

Co-authored-by: Thomas Grainger <[email protected]>
miss-islington added a commit that referenced this pull request Aug 28, 2021
@miss-islington @graingert
bpo-44962: Fix a race in WeakKeyDict, WeakValueDict and WeakSet when …
8aa64cc 
…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 <[email protected]>
(cherry picked from commit 206b21e)

Co-authored-by: Thomas Grainger <[email protected]>
@graingert graingert deleted the graingert:weakset-threadsafe branch Aug 28, 2021
ambv pushed a commit that referenced this pull request Aug 28, 2021
@miss-islington @graingert
bpo-44962: Fix a race in WeakKeyDict, WeakValueDict and WeakSet when …
166ad70 
…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 <[email protected]>
(cherry picked from commit 206b21e)

Co-authored-by: Thomas Grainger <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@bensimner bensimner

@ambv ambv

@pitrou pitrou

Assignees
No one assigned
Labels
CLA signed
Projects
None yet
Milestone
No milestone
6 participants
@graingert @miss-islington @bedevere-bot @ambv @bensimner @the-knights-who-say-ni