bpo-24484: Avoid race condition in multiprocessing cleanup by pitrou · Pull Request #2159 · python/cpython (original) (raw)
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 andprivacy statement. We’ll occasionally send you account related emails.
Already on GitHub?Sign in to your account
Conversation18 Commits4 Checks0 Files changed
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.Learn more about bidirectional Unicode characters
[ Show hidden characters]({{ revealButtonHref }})
The finalizer registry can be mutated while inspected by multiprocessing at process exit.
The finalizer registry can be mutated while inspected by multiprocessing at process exit.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
In general LGTM, but I have several questions.
| # list(_finalizer_registry) should be atomic, while |
| # list(_finalizer_registry.items()) is not. |
| keys = [key for key in list(_finalizer_registry) if f(key)] |
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This could be written also as
keys = list(_finalizer_registry)
keys = sorted(filter(f, keys), reverse=True)
if you prefer.
| @@ -3110,6 +3110,14 @@ class _TestFinalize(BaseTestCase): |
|---|
| ALLOWED_TYPES = ('processes',) |
| def setUp(self): |
| self.registry_backup = util._finalizer_registry.copy() |
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is this needed for every test or just for test_thread_safety?
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Just for test_thread_safety, but it was easier to put it here.
| threads.append(t) |
|---|
| time.sleep(4.0) # Wait a bit to trigger race condition |
| finish = True |
| for t in threads: |
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Maybe use test.support.start_threads() for running and finalizing threads?
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks, I will take a look.
| def make_finalizers(): |
|---|
| nonlocal exc |
| d = {} |
| threshold = 60 |
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is it used?
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
No, it's a leftover from a previous version of the test :-) Will remove.
| def run_finalizers(): |
| nonlocal exc |
| while not finish: |
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Maybe add and not exc?
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, perhaps.
| try: |
|---|
| # Old Foo's get gradually replaced and later |
| # collected by the GC (because of the cyclic ref) |
| d[random.getrandbits(5)] = {Foo() for i in range(10)} |
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why a set?
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I figured it would make deallocation order less deterministic than a list.
| while not finish: |
|---|
| try: |
| # Old Foo's get gradually replaced and later |
| # collected by the GC (because of the cyclic ref) |
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm wondering how much objects can create a fast interpreter (e.g. PyPy) on fast computer before hitting a bug or exhausting the test time.
| # is running (either by a GC run or by another thread). |
|---|
| # list(_finalizer_registry) should be atomic, while |
| # list(_finalizer_registry.items()) is not. |
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actually it is possible to make list(dict.items()) almost atomic (except allocating new tuples) or even truly atomic. Is it worth?
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
In a separate PR if you like, but this fix needs to be backported anyway.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Older versions of Python may need different fix or test due to different implementations of dict and finalization.
Older versions of Python may need different fix or test due to different implementations of dict and finalization.
That's quite likely. I can't think of a universal solution that wouldn't add a ton of complication, though.
(one possible complication would be to use one of the WeakDicts, which already has sophisticated logic to avoid race conditions)
I'm gonna merge this and see how the backports go.
pitrou added a commit to pitrou/cpython that referenced this pull request
- bpo-24484: Avoid race condition in multiprocessing cleanup
The finalizer registry can be mutated while inspected by multiprocessing at process exit.
Use test.support.start_threads()
Add Misc/NEWS. (cherry picked from commit 1eb6c00)
pitrou added a commit to pitrou/cpython that referenced this pull request
- bpo-24484: Avoid race condition in multiprocessing cleanup
The finalizer registry can be mutated while inspected by multiprocessing at process exit.
Use test.support.start_threads()
Add Misc/NEWS. (cherry picked from commit 1eb6c00)
pitrou added a commit to pitrou/cpython that referenced this pull request
- bpo-24484: Avoid race condition in multiprocessing cleanup
The finalizer registry can be mutated while inspected by multiprocessing at process exit.
Use test.support.start_threads()
Add Misc/NEWS. (cherry picked from commit 1eb6c00)
pitrou added a commit that referenced this pull request
- bpo-24484: Avoid race condition in multiprocessing cleanup
The finalizer registry can be mutated while inspected by multiprocessing at process exit.
Use test.support.start_threads()
Add Misc/NEWS. (cherry picked from commit 1eb6c00)
pitrou added a commit that referenced this pull request
- bpo-24484: Avoid race condition in multiprocessing cleanup
The finalizer registry can be mutated while inspected by multiprocessing at process exit.
Use test.support.start_threads()
Add Misc/NEWS. (cherry picked from commit 1eb6c00)
pitrou added a commit that referenced this pull request
- bpo-24484: Avoid race condition in multiprocessing cleanup
The finalizer registry can be mutated while inspected by multiprocessing at process exit.
Use test.support.start_threads()
Add Misc/NEWS. (cherry picked from commit 1eb6c00)
Labels
An unexpected behavior, bug, or error