bpo-24484: Avoid race condition in multiprocessing cleanup #2159
Conversation
The finalizer registry can be mutated while inspected by multiprocessing at process exit.
pitrou
added
needs backport to 2.7
type-bug
|
|
||
| # list(_finalizer_registry) should be atomic, while | ||
| # list(_finalizer_registry.items()) is not. | ||
| keys = [key for key in list(_finalizer_registry) if f(key)] |
There was a problem hiding this comment.
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() | |||
There was a problem hiding this comment.
Is this needed for every test or just for test_thread_safety?
There was a problem hiding this comment.
Just for test_thread_safety, but it was easier to put it here.
Lib/test/_test_multiprocessing.py
Outdated
| threads.append(t) | ||
| time.sleep(4.0) # Wait a bit to trigger race condition | ||
| finish = True | ||
| for t in threads: |
There was a problem hiding this comment.
Maybe use test.support.start_threads() for running and finalizing threads?
There was a problem hiding this comment.
Thanks, I will take a look.
Lib/test/_test_multiprocessing.py
Outdated
| def make_finalizers(): | ||
| nonlocal exc | ||
| d = {} | ||
| threshold = 60 |
There was a problem hiding this comment.
No, it's a leftover from a previous version of the test :-) Will remove.
|
|
||
| def run_finalizers(): | ||
| nonlocal exc | ||
| while not finish: |
There was a problem hiding this comment.
Maybe add and not exc?
| 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)} |
There was a problem hiding this comment.
I figured it would make deallocation order less deterministic than a list.
Lib/test/_test_multiprocessing.py
Outdated
| while not finish: | ||
| try: | ||
| # Old Foo's get gradually replaced and later | ||
| # collected by the GC (because of the cyclic ref) |
There was a problem hiding this comment.
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. |
There was a problem hiding this comment.
Actually it is possible to make list(dict.items()) almost atomic (except allocating new tuples) or even truly atomic. Is it worth?
There was a problem hiding this comment.
In a separate PR if you like, but this fix needs to be backported anyway.
That's quite likely. I can't think of a universal solution that wouldn't add a ton of complication, though. |
|
I'm gonna merge this and see how the backports go. |
…honGH-2159) * 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)
…honGH-2159) * 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)
…honGH-2159) * 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)
The finalizer registry can be mutated while inspected by multiprocessing at process exit.