From 2b93f5224216d10f8119373e72b5c2b3984e0af6 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Jakub=20Kul=C3=ADk?= Date: Sun, 17 Dec 2023 22:34:57 +0100 Subject: [PATCH] gh-113117: Support posix_spawn in subprocess.Popen with close_fds=True (#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] --- Doc/library/os.rst | 15 +++++++++-- Doc/whatsnew/3.13.rst | 26 +++++++++++++++++++ Lib/subprocess.py | 11 +++++--- Lib/test/test_subprocess.py | 5 ++++ ...-12-16-10-58-34.gh-issue-113117.0zF7bH.rst | 4 +++ Modules/posixmodule.c | 24 +++++++++++++++++ configure | 6 +++++ configure.ac | 1 + pyconfig.h.in | 4 +++ 9 files changed, 91 insertions(+), 5 deletions(-) create mode 100644 Misc/NEWS.d/next/Library/2023-12-16-10-58-34.gh-issue-113117.0zF7bH.rst diff --git a/Doc/library/os.rst b/Doc/library/os.rst index a079f1fa604..1138cc1f249 100644 --- a/Doc/library/os.rst +++ b/Doc/library/os.rst @@ -4601,10 +4601,17 @@ written in Python, such as a mail server's external command delivery program. Performs ``os.dup2(fd, new_fd)``. + .. data:: POSIX_SPAWN_CLOSEFROM + + (``os.POSIX_SPAWN_CLOSEFROM``, *fd*) + + Performs ``os.closerange(fd, INF)``. + These tuples correspond to the C library :c:func:`!posix_spawn_file_actions_addopen`, - :c:func:`!posix_spawn_file_actions_addclose`, and - :c:func:`!posix_spawn_file_actions_adddup2` API calls used to prepare + :c:func:`!posix_spawn_file_actions_addclose`, + :c:func:`!posix_spawn_file_actions_adddup2`, and + :c:func:`!posix_spawn_file_actions_addclosefrom_np` API calls used to prepare for the :c:func:`!posix_spawn` call itself. The *setpgroup* argument will set the process group of the child to the value @@ -4649,6 +4656,10 @@ written in Python, such as a mail server's external command delivery program. .. versionchanged:: 3.13 *env* parameter accepts ``None``. + .. versionchanged:: 3.13 + ``os.POSIX_SPAWN_CLOSEFROM`` is available on platforms where + :c:func:`!posix_spawn_file_actions_addclosefrom_np` exists. + .. availability:: Unix, not Emscripten, not WASI. .. function:: posix_spawnp(path, argv, env, *, file_actions=None, \ diff --git a/Doc/whatsnew/3.13.rst b/Doc/whatsnew/3.13.rst index 4af023566ff..2c869cbe113 100644 --- a/Doc/whatsnew/3.13.rst +++ b/Doc/whatsnew/3.13.rst @@ -293,6 +293,11 @@ os process use the current process environment. (Contributed by Jakub Kulik in :gh:`113119`.) +* :func:`os.posix_spawn` gains an :attr:`os.POSIX_SPAWN_CLOSEFROM` attribute for + use in ``file_actions=`` on platforms that support + :c:func:`!posix_spawn_file_actions_addclosefrom_np`. + (Contributed by Jakub Kulik in :gh:`113117`.) + pathlib ------- @@ -342,6 +347,21 @@ sqlite3 object is not :meth:`closed ` explicitly. (Contributed by Erlend E. Aasland in :gh:`105539`.) +subprocess +---------- + +* The :mod:`subprocess` module now uses the :func:`os.posix_spawn` function in + more situations. Notably in the default case of ``close_fds=True`` on more + recent versions of platforms including Linux, FreeBSD, and Solaris where the + C library provides :c:func:`!posix_spawn_file_actions_addclosefrom_np`. + On Linux this should perform similar to our existing Linux :c:func:`!vfork` + based code. A private control knob :attr:`!subprocess._USE_POSIX_SPAWN` can + be set to ``False`` if you need to force :mod:`subprocess` not to ever use + :func:`os.posix_spawn`. Please report your reason and platform details in + the CPython issue tracker if you set this so that we can improve our API + selection logic for everyone. + (Contributed by Jakub Kulik in :gh:`113117`.) + sys --- @@ -415,6 +435,12 @@ Optimizations * :func:`textwrap.indent` is now ~30% faster than before for large input. (Contributed by Inada Naoki in :gh:`107369`.) +* The :mod:`subprocess` module uses :func:`os.posix_spawn` in more situations + including the default where ``close_fds=True`` on many modern platforms. This + should provide a noteworthy performance increase launching processes on + FreeBSD and Solaris. See the ``subprocess`` section above for details. + (Contributed by Jakub Kulik in :gh:`113117`.) + Deprecated ========== diff --git a/Lib/subprocess.py b/Lib/subprocess.py index 1919ea4bdde..d5bd9a9e31a 100644 --- a/Lib/subprocess.py +++ b/Lib/subprocess.py @@ -748,6 +748,7 @@ def _use_posix_spawn(): # guarantee the given libc/syscall API will be used. _USE_POSIX_SPAWN = _use_posix_spawn() _USE_VFORK = True +_HAVE_POSIX_SPAWN_CLOSEFROM = hasattr(os, 'POSIX_SPAWN_CLOSEFROM') class Popen: @@ -1751,7 +1752,7 @@ class Popen: errread, errwrite) - def _posix_spawn(self, args, executable, env, restore_signals, + def _posix_spawn(self, args, executable, env, restore_signals, close_fds, p2cread, p2cwrite, c2pread, c2pwrite, errread, errwrite): @@ -1777,6 +1778,10 @@ class Popen: ): if fd != -1: file_actions.append((os.POSIX_SPAWN_DUP2, fd, fd2)) + + if close_fds: + file_actions.append((os.POSIX_SPAWN_CLOSEFROM, 3)) + if file_actions: kwargs['file_actions'] = file_actions @@ -1824,7 +1829,7 @@ class Popen: if (_USE_POSIX_SPAWN and os.path.dirname(executable) and preexec_fn is None - and not close_fds + and (not close_fds or _HAVE_POSIX_SPAWN_CLOSEFROM) and not pass_fds and cwd is None and (p2cread == -1 or p2cread > 2) @@ -1836,7 +1841,7 @@ class Popen: and gids is None and uid is None and umask < 0): - self._posix_spawn(args, executable, env, restore_signals, + self._posix_spawn(args, executable, env, restore_signals, close_fds, p2cread, p2cwrite, c2pread, c2pwrite, errread, errwrite) diff --git a/Lib/test/test_subprocess.py b/Lib/test/test_subprocess.py index 5eeea54fd55..6d3228bf92f 100644 --- a/Lib/test/test_subprocess.py +++ b/Lib/test/test_subprocess.py @@ -3348,6 +3348,7 @@ class POSIXProcessTestCase(BaseTestCase): @unittest.skipIf(not sysconfig.get_config_var("HAVE_VFORK"), "vfork() not enabled by configure.") @mock.patch("subprocess._fork_exec") + @mock.patch("subprocess._USE_POSIX_SPAWN", new=False) def test__use_vfork(self, mock_fork_exec): self.assertTrue(subprocess._USE_VFORK) # The default value regardless. mock_fork_exec.side_effect = RuntimeError("just testing args") @@ -3366,9 +3367,13 @@ class POSIXProcessTestCase(BaseTestCase): @unittest.skipIf(not sysconfig.get_config_var("HAVE_VFORK"), "vfork() not enabled by configure.") @unittest.skipIf(sys.platform != "linux", "Linux only, requires strace.") + @mock.patch("subprocess._USE_POSIX_SPAWN", new=False) def test_vfork_used_when_expected(self): # This is a performance regression test to ensure we default to using # vfork() when possible. + # Technically this test could pass when posix_spawn is used as well + # because libc tends to implement that internally using vfork. But + # that'd just be testing a libc+kernel implementation detail. strace_binary = "/usr/bin/strace" # The only system calls we are interested in. strace_filter = "--trace=clone,clone2,clone3,fork,vfork,exit,exit_group" diff --git a/Misc/NEWS.d/next/Library/2023-12-16-10-58-34.gh-issue-113117.0zF7bH.rst b/Misc/NEWS.d/next/Library/2023-12-16-10-58-34.gh-issue-113117.0zF7bH.rst new file mode 100644 index 00000000000..718226a0021 --- /dev/null +++ b/Misc/NEWS.d/next/Library/2023-12-16-10-58-34.gh-issue-113117.0zF7bH.rst @@ -0,0 +1,4 @@ +The :mod:`subprocess` module can now use the :func:`os.posix_spawn` function +with ``close_fds=True`` on platforms where +``posix_spawn_file_actions_addclosefrom_np`` is available. +Patch by Jakub Kulik. diff --git a/Modules/posixmodule.c b/Modules/posixmodule.c index 2dc5d7d81db..8ffe0f5de1e 100644 --- a/Modules/posixmodule.c +++ b/Modules/posixmodule.c @@ -6834,6 +6834,9 @@ enum posix_spawn_file_actions_identifier { POSIX_SPAWN_OPEN, POSIX_SPAWN_CLOSE, POSIX_SPAWN_DUP2 +#ifdef HAVE_POSIX_SPAWN_FILE_ACTIONS_ADDCLOSEFROM_NP + ,POSIX_SPAWN_CLOSEFROM +#endif }; #if defined(HAVE_SCHED_SETPARAM) || defined(HAVE_SCHED_SETSCHEDULER) || defined(POSIX_SPAWN_SETSCHEDULER) || defined(POSIX_SPAWN_SETSCHEDPARAM) @@ -7074,6 +7077,24 @@ parse_file_actions(PyObject *file_actions, } break; } +#ifdef HAVE_POSIX_SPAWN_FILE_ACTIONS_ADDCLOSEFROM_NP + case POSIX_SPAWN_CLOSEFROM: { + int fd; + if (!PyArg_ParseTuple(file_action, "Oi" + ";A closefrom file_action tuple must have 2 elements", + &tag_obj, &fd)) + { + goto fail; + } + errno = posix_spawn_file_actions_addclosefrom_np(file_actionsp, + fd); + if (errno) { + posix_error(); + goto fail; + } + break; + } +#endif default: { PyErr_SetString(PyExc_TypeError, "Unknown file_actions identifier"); @@ -16774,6 +16795,9 @@ all_ins(PyObject *m) if (PyModule_AddIntConstant(m, "POSIX_SPAWN_OPEN", POSIX_SPAWN_OPEN)) return -1; if (PyModule_AddIntConstant(m, "POSIX_SPAWN_CLOSE", POSIX_SPAWN_CLOSE)) return -1; if (PyModule_AddIntConstant(m, "POSIX_SPAWN_DUP2", POSIX_SPAWN_DUP2)) return -1; +#ifdef HAVE_POSIX_SPAWN_FILE_ACTIONS_ADDCLOSEFROM_NP + if (PyModule_AddIntMacro(m, POSIX_SPAWN_CLOSEFROM)) return -1; +#endif #endif #if defined(HAVE_SPAWNV) || defined (HAVE_RTPSPAWN) diff --git a/configure b/configure index 668a0efd77d..7e50abc29d0 100755 --- a/configure +++ b/configure @@ -17779,6 +17779,12 @@ if test "x$ac_cv_func_posix_spawnp" = xyes then : printf "%s\n" "#define HAVE_POSIX_SPAWNP 1" >>confdefs.h +fi +ac_fn_c_check_func "$LINENO" "posix_spawn_file_actions_addclosefrom_np" "ac_cv_func_posix_spawn_file_actions_addclosefrom_np" +if test "x$ac_cv_func_posix_spawn_file_actions_addclosefrom_np" = xyes +then : + printf "%s\n" "#define HAVE_POSIX_SPAWN_FILE_ACTIONS_ADDCLOSEFROM_NP 1" >>confdefs.h + fi ac_fn_c_check_func "$LINENO" "pread" "ac_cv_func_pread" if test "x$ac_cv_func_pread" = xyes diff --git a/configure.ac b/configure.ac index 020553abd71..e064848af9e 100644 --- a/configure.ac +++ b/configure.ac @@ -4757,6 +4757,7 @@ AC_CHECK_FUNCS([ \ lockf lstat lutimes madvise mbrtowc memrchr mkdirat mkfifo mkfifoat \ mknod mknodat mktime mmap mremap nice openat opendir pathconf pause pipe \ pipe2 plock poll posix_fadvise posix_fallocate posix_spawn posix_spawnp \ + posix_spawn_file_actions_addclosefrom_np \ pread preadv preadv2 pthread_condattr_setclock pthread_init pthread_kill \ pwrite pwritev pwritev2 readlink readlinkat readv realpath renameat \ rtpSpawn sched_get_priority_max sched_rr_get_interval sched_setaffinity \ diff --git a/pyconfig.h.in b/pyconfig.h.in index 9c429c03722..d8a9f68951a 100644 --- a/pyconfig.h.in +++ b/pyconfig.h.in @@ -905,6 +905,10 @@ /* Define to 1 if you have the `posix_spawnp' function. */ #undef HAVE_POSIX_SPAWNP +/* Define to 1 if you have the `posix_spawn_file_actions_addclosefrom_np' + function. */ +#undef HAVE_POSIX_SPAWN_FILE_ACTIONS_ADDCLOSEFROM_NP + /* Define to 1 if you have the `pread' function. */ #undef HAVE_PREAD