-
-
Notifications
You must be signed in to change notification settings - Fork 30.1k
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
Add support for posix_spawn call in subprocess.Popen with close_fds=True #113117
Comments
On macOS something this can be accomplished through
|
I like your PR and think it makes sense. And would welcome another one for macOS's I'm curious what the fall-out from making this change could be. We may not actually want to use posix_spawn on Linux by default. Doing some quick testing with this PR, it appears a little slower than our vfork code:
That was run on Ubuntu 22.04. (turning off both While performance is still "good", and the time difference appears constant regardless of tweaks to the process memory footprint or number of open file descriptors. It's still a 20% performance hit vs our Linux default subprocess vfork code-path. Performance of subprocess using posix_spawn presumably depends on the libc implementation. Replacing the This is something to keep an eye on. We could enhance the conditional logic on Linux used to determine when to use posix_spawn or not if desired. I don't know if that is worth doing. |
Let's use #109154 for the macOS part. That issue does not have a PR yet, but implementing this should be too hard. |
I see - sadly, I cannot help much with that as I don't have access to Mac, and it's a little bit more complex than just calling a different function - the Mac API seems more complicated (but also potentially more powerful).
Interesting - I guess we can always modify it to prefer |
I'll implement the macOS part in #109154 once your PR is resolved, either by emulating the Linux API or by exposing something closer to the native API. |
#113118) Add support for `os.POSIX_SPAWN_CLOSEFROM` and `posix_spawn_file_actions_addclosefrom_np` and have the `subprocess` module use them when available. This means `posix_spawn` can now be used in the default `close_fds=True` situation on many platforms. Co-authored-by: Gregory P. Smith [Google LLC] <greg@krypto.org>
Thanks a lot for this contribution! Merged. It'll be interesting to see how this works out. The buildbots are all happy with it. The performance being 5-10% lower than our vfork codepath on Linux is annoying only because I bothered to measure it, it shouldn't be a big deal, but if it turns out to cause anyone issues we can revisit the Linux default behavior and just do that within I'm not 100% confident in libc posix_spawn implementations getting everything right, at least on Linux, given the history of problems in much older glibc versions. But it sounds like recent versions are okay. I hope that holds up in practice. I don't know if We'll use other issues to track the macOS things, as well as the potential for posix_spawn |
PR #113118 breaks macOS framework builds such as those used by the python.org installer:
Note the comments elsewhere in posixmodule.c such as at line 1582:
|
…ds=True (python#113118) Add support for `os.POSIX_SPAWN_CLOSEFROM` and `posix_spawn_file_actions_addclosefrom_np` and have the `subprocess` module use them when available. This means `posix_spawn` can now be used in the default `close_fds=True` situation on many platforms. Co-authored-by: Gregory P. Smith [Google LLC] <greg@krypto.org>
…ds=True (python#113118) Add support for `os.POSIX_SPAWN_CLOSEFROM` and `posix_spawn_file_actions_addclosefrom_np` and have the `subprocess` module use them when available. This means `posix_spawn` can now be used in the default `close_fds=True` situation on many platforms. Co-authored-by: Gregory P. Smith [Google LLC] <greg@krypto.org>
…ds=True (python#113118) Add support for `os.POSIX_SPAWN_CLOSEFROM` and `posix_spawn_file_actions_addclosefrom_np` and have the `subprocess` module use them when available. This means `posix_spawn` can now be used in the default `close_fds=True` situation on many platforms. Co-authored-by: Gregory P. Smith [Google LLC] <greg@krypto.org>
Feature or enhancement
Proposal:
One of the reasons
posix_spawn
is rarely used with subprocess functions is that it doesn't supportclose_fds=True
(which is the default value).Previously, it was deemed infeasible to support this (#79718 (comment)), but since then,
posix_spawn_file_actions_addclosefrom_np()
has become available on many systems (I know about Solaris, Linux and FreeBSD), which should make it pretty simple to support it.Has this already been discussed elsewhere?
No response given
Links to previous discussion of this feature:
These two are related:
#79718 was the original issue for using os.posix_spawn() in subprocess.
#86904 proposed changing close_fds to default to False, so posix_spawn() could be used more often
Linked PRs
The text was updated successfully, but these errors were encountered: