GeezerVison is showing github.com — Add support for posix_spawn call in subprocess.Popen with close_fds=True · Issue #113117 · python/cpython
Add support for posix_spawn call in subprocess.Popen with close_fds=True #113117DescriptionFeature or enhancement
Proposal:
One of the reasons posix_spawn is rarely used with subprocess functions is that it doesn't support close_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
Reacted by Gregory P. Smith and Sam James Activitychanged the title [-]Add support for posix_spawn call with close_fds=True[/-] [+]Add support for posix_spawn call in subprocess.Popen with close_fds=True[/+] on Dec 14, 2023ronaldoussoren commented on Dec 14, 2023 On macOS something this can be accomplished through posix_spawn_file_actions_addinherit_np:
Normally, for posix_spawn(2) and posix_spawnp(2), all file descriptors are inherited from the parent process into the spawned process, except for those explicitly
marked as close-on-exec. However if the flag POSIX_SPAWN_CLOEXEC_DEFAULT is set, then during the spawn operation, all pre-existing file descriptors in the parent
process are treated as if they had been marked close-on-exec i.e. none of them are automatically inherited. See posix_spawnattr_setflags(3). Only file
descriptors explicitly manipulated via file_actions are made available in the spawned process. In that case, posix_spawn_file_actions_addinherit_np() can be used
to make specific pre-existing file descriptors from the parent process be available in the spawned process.
Reacted by Gregory P. Smith gpshead commented on Dec 15, 2023 I like your PR and think it makes sense. And would welcome another one for macOS's _np API flavor as well.
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:
bin/python3 -m timeit --setup 'import os; import subprocess; subprocess._HAVE_POSIX_SPAWN_CLOSEFROM=False; memory=1_000_000 * [os.open("/dev/null", 0) for _ in range(50)]' 'subprocess.run(["/bin/true"])'
1000 loops, best of 5: 290 usec per loop
bin/python3 -m timeit --setup 'import os; import subprocess; subprocess._HAVE_POSIX_SPAWN_CLOSEFROM=True; memory=1_000_000 * [os.open("/dev/null", 0) for _ in range(50)]' 'subprocess.run(["/bin/true"])'
1000 loops, best of 5: 350 usec per loop
That was run on Ubuntu 22.04. (turning off both posix_spawn & vfork in the above for a traditional fork takes 5600 usec)
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 subprocess.py os.posix_spawn and _posixsubprocess.fork_exec calls with a dummy, the pure Python overhead is clearly larger for the fork_exec vfork case (16us) than it is for the posix_spawn case (4us). That points to the internals of my systems glibc posix_spawn implementation just being a bit slower (which I believe just uses vfork itself).
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. ronaldoussoren commented on Dec 15, 2023 Let's use #109154 for the macOS part. That issue does not have a PR yet, but implementing this should be too hard. Reacted by Gregory P. Smith kulikjak commented on Dec 16, 2023 ContributorAuthor More actions
On macOS something this can be accomplished through posix_spawn_file_actions_addinherit_np:
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).
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.
Interesting - I guess we can always modify it to prefer vfork over posix_spawn on Linux when it's available. On other platforms, I think that posix_spawn should always be better than standard fork/exec. ronaldoussoren commented on Dec 16, 2023
On macOS something this can be accomplished through posix_spawn_file_actions_addinherit_np:
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).
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. added a commit that references this issue on Dec 17, 2023gh-113117: Support posix_spawn in subprocess.Popen with close_fds=True ( gpshead commented on Dec 17, 2023 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 subprocess._use_posix_spawn() in a future PR.
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 posix_spawn_file_actions_addclosefrom_np() existed on Linux when we were originally getting os.posix_spawn support working (a quick search suggests it may be fairly new to Linux?), but regardless it still is not meaningfully documented on Linux despite having been implemented (ugh). So I'm not surprised it was overlooked. The Solaris and FreeBSD docs help.
We'll use other issues to track the macOS things, as well as the potential for posix_spawn chdir support (which I think while available as a non-portable _np extension on some platforms is becoming a standard and will drop the _np suffix in some distant future? https://www.austingroupbugs.net/view.php?id=1208) Reacted by Jakub Kulík Reacted by Jakub Kulík ned-deily commented on Dec 19, 2023 PR #113118 breaks macOS framework builds such as those used by the python.org installer:
$ ./configure --enable-framework && make -j
[...]
./Modules/posixmodule.c:7170:19: error: use of undeclared identifier 'environ'
envlist = environ;
^
./Modules/posixmodule.c:7238:31: error: use of undeclared identifier 'environ'
if (envlist && envlist != environ) {
^
Note the comments elsewhere in posixmodule.c such as at line 1582:
#elif defined(WITH_NEXT_FRAMEWORK) || (defined(__APPLE__) && defined(Py_ENABLE_SHARED))
/* environ is not accessible as an extern in a shared object on OSX; use
_NSGetEnviron to resolve it. The value changes if you add environment
variables between calls to Py_Initialize, so don't cache the value. */
e = *_NSGetEnviron();
gpshead commented on Dec 19, 2023 this was not what broke the macos build. that was #113119. see #113268 for a fix.
MetadataMetadataDevelopmentNo branches or pull requestsIssue actions
|