Uh oh!
There was an error while loading.Please reload this page.
- Notifications
You must be signed in to change notification settings - Fork32.1k
gh-77377: Ensure multiprocessing SemLock is valid for spawn-based Process before serializing it#107275
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 ourterms of service andprivacy statement. We’ll occasionally send you account related emails.
Already on GitHub?Sign in to your account
Uh oh!
There was an error while loading.Please reload this page.
Conversation
bedevere-bot commentedJul 25, 2023
Most changes to Pythonrequire a NEWS entry. Please add it using theblurb_it web app or theblurb command-line tool. |
Lib/test/_test_multiprocessing.py Outdated
| @@ -6155,3 +6155,9 @@ class SemLock(_multiprocessing.SemLock): | |||
| name = f'test_semlock_subclass-{os.getpid()}' | |||
| s = SemLock(1, 0, 10, name, False) | |||
| _multiprocessing.sem_unlink(name) | |||
| def test_semlock_mixed_context(self): | |||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
Does this test actually need to be Linux-only? (seeSemLockTests definition above)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
This is indeed also relevant on macos. Moved the test to be able to test it there.
On windows, there is only spawn, so there is no way to get "mixed" context.
Lib/test/_test_multiprocessing.py Outdated
| def test_semlock_mixed_context(self): | ||
| queue = multiprocessing.get_context("fork").Queue() | ||
| p = multiprocessing.get_context("spawn").Process(target=close_queue, args=(queue,)) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
Do we also want to test with "forkserver" method?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
Good catch! Added it.
It now ensure that all cases work. Let me know if that properly covers all the cases you had in mind.
Lib/test/_test_multiprocessing.py Outdated
| @@ -5421,6 +5421,24 @@ def test_preload_resources(self): | |||
| print(err) | |||
| self.fail("failed spawning forkserver or grandchild") | |||
| @unittest.skipIf(sys.platform == "win32", "Only Spawn on windows so no risk of mixing") | |||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
Style nit, but can you try to wrap the lines below around ~80 characters max?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
Updated!
Thanks for the update on the blurb, I wasn't sure what to put there
bedevere-bot commentedAug 23, 2023
GH-108377 is a backport of this pull request to the3.12 branch. |
…ed Process before serializing it (pythonGH-107275)Ensure multiprocessing SemLock is valid for spawn Process before serializing it.Creating a multiprocessing SemLock with a fork context, and then trying to pass it to a spawn-created Process, would segfault if not detected early.---------(cherry picked from commit1700d34)Co-authored-by: albanD <desmaison.alban@gmail.com>Co-authored-by: blurb-it[bot] <43283697+blurb-it[bot]@users.noreply.github.com>Co-authored-by: Antoine Pitrou <pitrou@free.fr>
bedevere-bot commentedAug 23, 2023
GH-108378 is a backport of this pull request to the3.11 branch. |
…ed Process before serializing it (pythonGH-107275)Ensure multiprocessing SemLock is valid for spawn Process before serializing it.Creating a multiprocessing SemLock with a fork context, and then trying to pass it to a spawn-created Process, would segfault if not detected early.---------(cherry picked from commit1700d34)Co-authored-by: albanD <desmaison.alban@gmail.com>Co-authored-by: blurb-it[bot] <43283697+blurb-it[bot]@users.noreply.github.com>Co-authored-by: Antoine Pitrou <pitrou@free.fr>
…sed Process before serializing it (GH-107275) (#108378)gh-77377: Ensure multiprocessing SemLock is valid for spawn-based Process before serializing it (GH-107275)Ensure multiprocessing SemLock is valid for spawn Process before serializing it.Creating a multiprocessing SemLock with a fork context, and then trying to pass it to a spawn-created Process, would segfault if not detected early.---------(cherry picked from commit1700d34)Co-authored-by: albanD <desmaison.alban@gmail.com>Co-authored-by: blurb-it[bot] <43283697+blurb-it[bot]@users.noreply.github.com>Co-authored-by: Antoine Pitrou <pitrou@free.fr>
…sed Process before serializing it (GH-107275) (#108377)gh-77377: Ensure multiprocessing SemLock is valid for spawn-based Process before serializing it (GH-107275)Ensure multiprocessing SemLock is valid for spawn Process before serializing it.Creating a multiprocessing SemLock with a fork context, and then trying to pass it to a spawn-created Process, would segfault if not detected early.---------(cherry picked from commit1700d34)Co-authored-by: albanD <desmaison.alban@gmail.com>Co-authored-by: blurb-it[bot] <43283697+blurb-it[bot]@users.noreply.github.com>Co-authored-by: Antoine Pitrou <pitrou@free.fr>
…108568)gh-107275 introduced a regression where a SemLock would fail being passed along nested child processes, as the `is_fork_ctx` attribute would be left missing after the first deserialization.---------Co-authored-by: blurb-it[bot] <43283697+blurb-it[bot]@users.noreply.github.com>Co-authored-by: Antoine Pitrou <pitrou@free.fr>
… case (pythonGH-108568)pythongh-107275 introduced a regression where a SemLock would fail being passed along nested child processes, as the `is_fork_ctx` attribute would be left missing after the first deserialization.---------(cherry picked from commitadd8d45)Co-authored-by: albanD <desmaison.alban@gmail.com>Co-authored-by: blurb-it[bot] <43283697+blurb-it[bot]@users.noreply.github.com>Co-authored-by: Antoine Pitrou <pitrou@free.fr>
…e case (GH-108568) (#108692)gh-107275 introduced a regression where a SemLock would fail being passed along nested child processes, as the `is_fork_ctx` attribute would be left missing after the first deserialization.---------(cherry picked from commitadd8d45)Co-authored-by: albanD <desmaison.alban@gmail.com>Co-authored-by: blurb-it[bot] <43283697+blurb-it[bot]@users.noreply.github.com>Co-authored-by: Antoine Pitrou <pitrou@free.fr>
…e case (GH-108568) (#108691)gh-108520: Fix bad fork detection in nested multiprocessing use case (GH-108568)gh-107275 introduced a regression where a SemLock would fail being passed along nested child processes, as the `is_fork_ctx` attribute would be left missing after the first deserialization.---------(cherry picked from commitadd8d45)Co-authored-by: albanD <desmaison.alban@gmail.com>Co-authored-by: blurb-it[bot] <43283697+blurb-it[bot]@users.noreply.github.com>Co-authored-by: Antoine Pitrou <pitrou@free.fr>
Uh oh!
There was an error while loading.Please reload this page.
This fixes both the example in the initial report#77377 and other reports on the PyTorch repo.
Note that as far as I can tell, all objects that lead to segfault with mixed context are all caused by SemLock created with fork sent over a Spawn process. That's why it is the only object updated here and the only pair of ctx covered. Let me know if I'm mistaken and I can find a way to update other objects as well.
Also windows behavior is not changed here (and thus not tested).
Please let me know if the error message and/or the test are not appropriate and I can work on improving them.