From 7d489629d8301bb73ca4081b11f469db523fbc8e Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Jakub=20Kul=C3=ADk?= Date: Thu, 14 Dec 2023 15:52:40 +0100 Subject: [PATCH 1/9] Add support for os.POSIX_SPAWN_CLOSEFROM and posix_spawn_file_actions_addclosefrom_np --- Lib/subprocess.py | 11 ++++++++--- Modules/posixmodule.c | 24 ++++++++++++++++++++++++ configure.ac | 1 + pyconfig.h.in | 4 ++++ 4 files changed, 37 insertions(+), 3 deletions(-) diff --git a/Lib/subprocess.py b/Lib/subprocess.py index d6edd1a9807d1b..816ae30b8f7326 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 @@ def _get_handles(self, stdin, stdout, stderr): 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): @@ -1780,6 +1781,10 @@ def _posix_spawn(self, args, executable, env, restore_signals, ): 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 @@ -1827,7 +1832,7 @@ def _execute_child(self, args, executable, preexec_fn, close_fds, 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) @@ -1839,7 +1844,7 @@ def _execute_child(self, args, executable, preexec_fn, close_fds, 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/Modules/posixmodule.c b/Modules/posixmodule.c index ddbb4cd43babfc..1576a0e575bc64 100644 --- a/Modules/posixmodule.c +++ b/Modules/posixmodule.c @@ -6787,6 +6787,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) @@ -7027,6 +7030,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"); @@ -16723,6 +16744,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_AddIntConstant(m, "POSIX_SPAWN_CLOSEFROM", POSIX_SPAWN_CLOSEFROM)) return -1; +#endif #endif #if defined(HAVE_SPAWNV) || defined (HAVE_RTPSPAWN) diff --git a/configure.ac b/configure.ac index 020553abd71b4f..e064848af9ed1b 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 9c429c03722383..d8a9f68951afbd 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 From 81207b89d318a6f4ee4e4d4e3c7a37ccc257c2ce Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Jakub=20Kul=C3=ADk?= Date: Thu, 14 Dec 2023 15:54:34 +0100 Subject: [PATCH 2/9] Add documentation --- Doc/library/os.rst | 15 +++++++++++++-- 1 file changed, 13 insertions(+), 2 deletions(-) diff --git a/Doc/library/os.rst b/Doc/library/os.rst index 9d2a3d65069253..f7e9cf4d6f285c 100644 --- a/Doc/library/os.rst +++ b/Doc/library/os.rst @@ -4583,10 +4583,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 @@ -4628,6 +4635,10 @@ written in Python, such as a mail server's external command delivery program. .. versionadded:: 3.8 + .. 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, \ From 79d094e1bfa74021b81e514350eb2bb9ea7704b8 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Jakub=20Kul=C3=ADk?= Date: Thu, 14 Dec 2023 16:57:05 +0100 Subject: [PATCH 3/9] Add regenerated configure --- configure | 6 ++++++ 1 file changed, 6 insertions(+) diff --git a/configure b/configure index 668a0efd77db0e..7e50abc29d0c1a 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 From 6f171156a8cb97c610774a12b86378afa4b2fd84 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Jakub=20Kul=C3=ADk?= Date: Sat, 16 Dec 2023 10:49:59 +0100 Subject: [PATCH 4/9] use PyModule_AddIntMacro --- Modules/posixmodule.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/Modules/posixmodule.c b/Modules/posixmodule.c index 1576a0e575bc64..20369ab0adefa5 100644 --- a/Modules/posixmodule.c +++ b/Modules/posixmodule.c @@ -16745,7 +16745,7 @@ all_ins(PyObject *m) 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_AddIntConstant(m, "POSIX_SPAWN_CLOSEFROM", POSIX_SPAWN_CLOSEFROM)) return -1; + if (PyModule_AddIntMacro(m, POSIX_SPAWN_CLOSEFROM)) return -1; #endif #endif From 4a99eef561bcfd7df06f09ac1abce23ef9add2ad Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Jakub=20Kul=C3=ADk?= Date: Sat, 16 Dec 2023 10:59:36 +0100 Subject: [PATCH 5/9] Add NEWS fragment and whatsnew/3.13 entry --- Doc/whatsnew/3.13.rst | 5 +++++ .../Library/2023-12-16-10-58-34.gh-issue-113117.0zF7bH.rst | 4 ++++ 2 files changed, 9 insertions(+) create mode 100644 Misc/NEWS.d/next/Library/2023-12-16-10-58-34.gh-issue-113117.0zF7bH.rst diff --git a/Doc/whatsnew/3.13.rst b/Doc/whatsnew/3.13.rst index d599ba9ae6fac8..93a09c8c3f8371 100644 --- a/Doc/whatsnew/3.13.rst +++ b/Doc/whatsnew/3.13.rst @@ -383,6 +383,11 @@ Optimizations * :func:`textwrap.indent` is now ~30% faster than before for large input. (Contributed by Inada Naoki in :gh:`107369`.) +* 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. + (Contributed by Jakub Kulik in :gh:`113117`.) + Deprecated ========== 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 00000000000000..718226a0021efe --- /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. From cb496e3d10869364832ff1a759bb7af2bda9d5cd Mon Sep 17 00:00:00 2001 From: "Gregory P. Smith [Google LLC]" Date: Sun, 17 Dec 2023 07:30:53 +0000 Subject: [PATCH 6/9] Have vfork specific tests disable posix_spawn. --- Lib/test/test_subprocess.py | 5 +++++ 1 file changed, 5 insertions(+) diff --git a/Lib/test/test_subprocess.py b/Lib/test/test_subprocess.py index 5eeea54fd55f1a..6d3228bf92f8ca 100644 --- a/Lib/test/test_subprocess.py +++ b/Lib/test/test_subprocess.py @@ -3348,6 +3348,7 @@ def exit_handler(): @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 @@ def test__use_vfork(self, mock_fork_exec): @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" From dfa43af55e65224a43e6a729af84d98dc8731fa3 Mon Sep 17 00:00:00 2001 From: "Gregory P. Smith" Date: Sun, 17 Dec 2023 12:32:19 -0800 Subject: [PATCH 7/9] Expand upon the What's New text. Gives a more useful description and mentions that it can be turned off but that we'd like to hear from anyone doing so. --- Doc/whatsnew/3.13.rst | 12 +++++++++--- 1 file changed, 9 insertions(+), 3 deletions(-) diff --git a/Doc/whatsnew/3.13.rst b/Doc/whatsnew/3.13.rst index 34a68f5835075f..6bd616abd4bb56 100644 --- a/Doc/whatsnew/3.13.rst +++ b/Doc/whatsnew/3.13.rst @@ -415,9 +415,15 @@ Optimizations * :func:`textwrap.indent` is now ~30% faster than before for large input. (Contributed by Inada Naoki in :gh:`107369`.) -* 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. +* The :mod:`subprocess` module may use the :func:`os.posix_spawn` function + in more situations. Notably in the default of ``close_fds=True`` + on platforms such as modern Linux 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 :func:`os.posix_spawn` not to be used. + Please report your reasons 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`.) From 4c96d421208cdb0432922d00736edfddfe8f9061 Mon Sep 17 00:00:00 2001 From: "Gregory P. Smith" Date: Sun, 17 Dec 2023 12:48:23 -0800 Subject: [PATCH 8/9] fix syntax typos in what's new (the joys of using the github web editor) --- Doc/whatsnew/3.13.rst | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/Doc/whatsnew/3.13.rst b/Doc/whatsnew/3.13.rst index 6bd616abd4bb56..b2d83fd066591a 100644 --- a/Doc/whatsnew/3.13.rst +++ b/Doc/whatsnew/3.13.rst @@ -419,7 +419,7 @@ Optimizations in more situations. Notably in the default of ``close_fds=True`` on platforms such as modern Linux 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. + 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 :func:`os.posix_spawn` not to be used. Please report your reasons and platform details in the CPython issue tracker From 654f63053aee162a6e59fa894b2cd8ee82a33a77 Mon Sep 17 00:00:00 2001 From: "Gregory P. Smith [Google LLC]" Date: Sun, 17 Dec 2023 21:11:19 +0000 Subject: [PATCH 9/9] Reword What's New, add os and subprocess headers. This describes the module changes in module sections and makes the Optimizations section brief, referring back up to the subprocess module. --- Doc/whatsnew/3.13.rst | 33 ++++++++++++++++++++++++--------- 1 file changed, 24 insertions(+), 9 deletions(-) diff --git a/Doc/whatsnew/3.13.rst b/Doc/whatsnew/3.13.rst index b2d83fd066591a..2c869cbe11396b 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,15 +435,10 @@ Optimizations * :func:`textwrap.indent` is now ~30% faster than before for large input. (Contributed by Inada Naoki in :gh:`107369`.) -* The :mod:`subprocess` module may use the :func:`os.posix_spawn` function - in more situations. Notably in the default of ``close_fds=True`` - on platforms such as modern Linux 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 :func:`os.posix_spawn` not to be used. - Please report your reasons and platform details in the CPython issue tracker - if you set this so that we can improve our API selection logic for everyone. +* 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`.)