Bug 916566 returnproc migration #1251
Merged
Conversation
This file contains 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
zmedico
force-pushed
the
bug_916566_returnproc_migration
branch
from
fff7884
to
fec465c
Compare
zmedico
force-pushed
the
bug_916566_returnproc_migration
branch
5 times, most recently
from
3e51fb6
to
933c540
Compare
zmedico
force-pushed
the
bug_916566_returnproc_migration
branch
6 times, most recently
from
3bf3797
to
c2556b1
Compare
zmedico
force-pushed
the
bug_916566_returnproc_migration
branch
7 times, most recently
from
89a84db
to
92e7e22
Compare
zmedico
force-pushed
the
bug_916566_returnproc_migration
branch
from
92e7e22
to
c789c7e
Compare
zmedico
force-pushed
the
bug_916566_returnproc_migration
branch
from
c789c7e
to
7bdb23c
Compare
Convert _send_fd_pipes BrokenPipeError to asyncio.CancelledError, in order to gracefully handle a concurrently terminated child process as in testAsynchronousLockWaitCancel. Even if the child terminated abnormally, then there is no harm in suppressing the exception here, since the child error should have gone to stderr. Bug: https://bugs.gentoo.org/923852 Signed-off-by: Zac Medico <zmedico@gentoo.org>
zmedico
force-pushed
the
bug_916566_returnproc_migration
branch
from
7bdb23c
to
97f27d6
Compare
I'm really curious about this part -- spawn should be quite a bit faster where |
thesamesam
approved these changes
Feb 7, 2024
Use the AsyncFunction create_pipe=False parameter to avoid issues in the pipe code triggered with the "spawn" multiprocessing start method when spawn uses multiprocessing.Process (bug 916566), since these jobs should inherit stdio streams and run in the foreground with no log. Also fix RetryForkExecutorTestCase to avoid pickling issues. Bug: https://bugs.gentoo.org/923854 Signed-off-by: Zac Medico <zmedico@gentoo.org>
Use multiprocessing.Process for returnproc, so that fork will stop being used when python makes "spawn" the default multiprocessing start method. Continue to use _start_fork when returnproc is not enabled, for backward compatibility. Ultimately, it can be removed at the same time as the returnpid parameter. The _setup_pipes_after_fork wrapper prevents a "Bad file descriptor" error by making fd_pipes inheritable on exec for bug 923755. ForkProcess does not handle this because its target function does not necessarily exec. Bug: https://bugs.gentoo.org/916566 Bug: https://bugs.gentoo.org/923755 Signed-off-by: Zac Medico <zmedico@gentoo.org>
zmedico
force-pushed
the
bug_916566_returnproc_migration
branch
from
97f27d6
to
1cd832d
Compare
Raise NotImplementedError if returnproc is enabled for anything other than the "depend" phase, since corresponding returnpid support has long been deprecated. Bug: https://bugs.gentoo.org/916566 Signed-off-by: Zac Medico <zmedico@gentoo.org>
Bug: https://bugs.gentoo.org/916566 Signed-off-by: Zac Medico <zmedico@gentoo.org>
Bug: https://bugs.gentoo.org/916566 Signed-off-by: Zac Medico <zmedico@gentoo.org>
All internal returnpid consumers have been migrated to use the new returnproc parameter instead. Bug: https://bugs.gentoo.org/916566 Signed-off-by: Zac Medico <zmedico@gentoo.org>
This essentially completes the implementation of bug 916566, eliminating os.fork() usage when "spawn" becomes the default multiprocessing start method. Bug: https://bugs.gentoo.org/916566 Signed-off-by: Zac Medico <zmedico@gentoo.org>
zmedico
force-pushed
the
bug_916566_returnproc_migration
branch
from
1cd832d
to
3e10368
Compare
The issue is that there is a lot of back-and-forth between the parent and child process during the child process setup. The parent needs to pickle any referenced objects and send them. ForkProcess needs to send fd_pipes just before the target function is called. It's slow for unit tests where a disproportionate number of processes are created and destroyed, but for normal operation the difference should be pretty negligible. |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Now includes a 3.12-dev ci job patched to set the multiprocessing start method to spawn, which took 7m 11s compared to 4m 16s for the 3.12-dev fork ci job.
Bug: https://bugs.gentoo.org/916566