From f638efa2761ce242723ef0526165793dc200f887 Mon Sep 17 00:00:00 2001 From: "Gregory P. Smith" Date: Sat, 27 May 2017 11:34:12 -0700 Subject: [PATCH 01/14] Use os module constants for os.register_at_fork. Adds the constants and updates all uses and documentation. --- Doc/c-api/sys.rst | 10 ++++++---- Doc/library/os.rst | 12 +++++++----- Lib/os.py | 5 +++++ Lib/random.py | 2 +- Lib/test/test_posix.py | 14 +++++++------- Modules/clinic/posixmodule.c.h | 10 +++++----- Modules/posixmodule.c | 10 +++++----- 7 files changed, 36 insertions(+), 27 deletions(-) diff --git a/Doc/c-api/sys.rst b/Doc/c-api/sys.rst index c6777d693363a7e..ac548b8729d29c4 100644 --- a/Doc/c-api/sys.rst +++ b/Doc/c-api/sys.rst @@ -49,10 +49,12 @@ Operating System Utilities .. c:function:: void PyOS_AfterFork_Child() - Function to update some internal state after a process fork. This - should be called from the child process after calling :c:func:`fork` - or any similar function that clones the current process. - Only available on systems where :c:func:`fork` is defined. + Function to update internal interpreter state after a process fork. + This must be called from the child process after calling :c:func:`fork`, + or any similar function that clones the current process, if there is + any chance the process will call back into the Python interpreter. + Only available on systems where :c:func:`fork` is defined as determined + by the ``HAVE_FORK`` C preprocessor define. .. versionadded:: 3.7 diff --git a/Doc/library/os.rst b/Doc/library/os.rst index 28921ad191fdcbb..c937a69cd666e30 100644 --- a/Doc/library/os.rst +++ b/Doc/library/os.rst @@ -3283,13 +3283,15 @@ written in Python, such as a mail server's external command delivery program. .. function:: register_at_fork(func, when) Register *func* as a function to be executed when a new child process - is forked. *when* is a string specifying at which point the function is + is forked. *when* is a constant specifying at which point the function is called and can take the following values: - * *"before"* means the function is called before forking a child process; - * *"parent"* means the function is called from the parent process after - forking a child process; - * *"child"* means the function is called from the child process. + * ``os.BEFORE_FORK`` means the function is called before forking a child + process. + * ``os.AFTER_FORK_PARENT`` means the function is called from the parent + process after forking a child process. + * ``os.AFTER_FORK_CHILD`` means the function is called from the child + process if control is expected to return to the Python interpreter. Functions registered for execution before forking are called in reverse registration order. Functions registered for execution diff --git a/Lib/os.py b/Lib/os.py index e293ecae7fd3a4e..47c6e97561548ba 100644 --- a/Lib/os.py +++ b/Lib/os.py @@ -94,6 +94,11 @@ def _get_exports_list(module): del _names +if _exists("register_at_fork"): + # Constants for use with os.register_at_fork()'s when parameter. + BEFORE_FORK = "before" + AFTER_FORK_CHILD = "child" + AFTER_FORK_PARENT = "parent" if _exists("_have_functions"): _globals = globals() diff --git a/Lib/random.py b/Lib/random.py index 52df7d8f74ba030..4188afd9042bed5 100644 --- a/Lib/random.py +++ b/Lib/random.py @@ -765,7 +765,7 @@ def _test(N=2000): getrandbits = _inst.getrandbits if hasattr(_os, "fork"): - _os.register_at_fork(_inst.seed, when='child') + _os.register_at_fork(_inst.seed, when=_os.AFTER_FORK_CHILD) if __name__ == '__main__': diff --git a/Lib/test/test_posix.py b/Lib/test/test_posix.py index a72f83c8dcfe9b2..596fd903e172616 100644 --- a/Lib/test/test_posix.py +++ b/Lib/test/test_posix.py @@ -189,19 +189,19 @@ def test_waitid(self): self.assertEqual(pid, res.si_pid) @unittest.skipUnless(hasattr(os, 'fork'), "test needs os.fork()") - def test_register_after_fork(self): + def test_register_at_fork(self): code = """if 1: import os r, w = os.pipe() fin_r, fin_w = os.pipe() - os.register_at_fork(lambda: os.write(w, b'A'), when='before') - os.register_at_fork(lambda: os.write(w, b'B'), when='before') - os.register_at_fork(lambda: os.write(w, b'C'), when='parent') - os.register_at_fork(lambda: os.write(w, b'D'), when='parent') - os.register_at_fork(lambda: os.write(w, b'E'), when='child') - os.register_at_fork(lambda: os.write(w, b'F'), when='child') + os.register_at_fork(lambda: os.write(w, b'A'), when=os.BEFORE_FORK) + os.register_at_fork(lambda: os.write(w, b'B'), when=os.BEFORE_FORK) + os.register_at_fork(lambda: os.write(w, b'C'), when=os.AFTER_FORK_PARENT) + os.register_at_fork(lambda: os.write(w, b'D'), when=os.AFTER_FORK_PARENT) + os.register_at_fork(lambda: os.write(w, b'E'), when=os.AFTER_FORK_CHILD) + os.register_at_fork(lambda: os.write(w, b'F'), when=os.AFTER_FORK_CHILD) pid = os.fork() if pid == 0: diff --git a/Modules/clinic/posixmodule.c.h b/Modules/clinic/posixmodule.c.h index 2c919e18795c7f0..62204d2267dc674 100644 --- a/Modules/clinic/posixmodule.c.h +++ b/Modules/clinic/posixmodule.c.h @@ -1836,11 +1836,11 @@ PyDoc_STRVAR(os_register_at_fork__doc__, " func\n" " Function or callable\n" " when\n" -" \'before\', \'child\' or \'parent\'\n" +" os.BEFORE_FORK, os.AFTER_FORK_CHILD, os.AFTER_FORK_PARENT\n" "\n" -"\'before\' callbacks are called in reverse order before forking.\n" -"\'child\' callbacks are called in order after forking, in the child process.\n" -"\'parent\' callbacks are called in order after forking, in the parent process."); +"os.BEFORE_FORK callbacks are called in reverse order before forking.\n" +"os.AFTER_FORK_CHILD callbacks are called in order after forking, in the child process.\n" +"os.AFTER_FORK_PARENT callbacks are called in order after forking, in the parent process."); #define OS_REGISTER_AT_FORK_METHODDEF \ {"register_at_fork", (PyCFunction)os_register_at_fork, METH_FASTCALL, os_register_at_fork__doc__}, @@ -6541,4 +6541,4 @@ os_getrandom(PyObject *module, PyObject **args, Py_ssize_t nargs, PyObject *kwna #ifndef OS_GETRANDOM_METHODDEF #define OS_GETRANDOM_METHODDEF #endif /* !defined(OS_GETRANDOM_METHODDEF) */ -/*[clinic end generated code: output=699e11c5579a104e input=a9049054013a1b77]*/ +/*[clinic end generated code: output=30f4881161078e93 input=a9049054013a1b77]*/ diff --git a/Modules/posixmodule.c b/Modules/posixmodule.c index be8a66dd5028e8f..2c3ecb5498bc315 100644 --- a/Modules/posixmodule.c +++ b/Modules/posixmodule.c @@ -5316,19 +5316,19 @@ os.register_at_fork Function or callable / when: str - 'before', 'child' or 'parent' + os.BEFORE_FORK, os.AFTER_FORK_CHILD, os.AFTER_FORK_PARENT Register a callable object to be called when forking. -'before' callbacks are called in reverse order before forking. -'child' callbacks are called in order after forking, in the child process. -'parent' callbacks are called in order after forking, in the parent process. +os.BEFORE_FORK callbacks are called in reverse order before forking. +os.AFTER_FORK_CHILD callbacks are called in order after forking, in the child process. +os.AFTER_FORK_PARENT callbacks are called in order after forking, in the parent process. [clinic start generated code]*/ static PyObject * os_register_at_fork_impl(PyObject *module, PyObject *func, const char *when) -/*[clinic end generated code: output=8943be81a644750c input=5fc05efa4d42eb84]*/ +/*[clinic end generated code: output=8943be81a644750c input=fde1c6bde63beb91]*/ { PyInterpreterState *interp; PyObject **lst; From ac7cd2b13d9183a8385919d39c573de8e1cfe8c5 Mon Sep 17 00:00:00 2001 From: "Gregory P. Smith" Date: Sat, 27 May 2017 16:10:31 -0700 Subject: [PATCH 02/14] Switch to keyword only arguments instead of consts This switches from using str constants or os module constants on a warn parameter to using named keyword only arguments specifying the intent of the callable being passed in. It avoids the need for string literals or new os module constants and is generally more readable. This is more similar to how the libc pthread_atfork() API works. --- Doc/library/os.rst | 24 ++++++++------- Lib/os.py | 6 ---- Lib/random.py | 2 +- Lib/test/test_posix.py | 22 ++++++++++---- Modules/clinic/posixmodule.c.h | 39 +++++++++++++----------- Modules/posixmodule.c | 55 +++++++++++++++++----------------- 6 files changed, 81 insertions(+), 67 deletions(-) diff --git a/Doc/library/os.rst b/Doc/library/os.rst index c937a69cd666e30..33899939b480a37 100644 --- a/Doc/library/os.rst +++ b/Doc/library/os.rst @@ -3280,18 +3280,20 @@ written in Python, such as a mail server's external command delivery program. subprocesses. -.. function:: register_at_fork(func, when) +.. function:: register_at_fork(*, before=None, after_in_parent=None, \ + after_in_child=None) - Register *func* as a function to be executed when a new child process - is forked. *when* is a constant specifying at which point the function is - called and can take the following values: + Register callables to be executed when a new child process is forked + using :func:`os.fork`. The parameters are optional and keyword-only. + Each specifies a different call point. - * ``os.BEFORE_FORK`` means the function is called before forking a child - process. - * ``os.AFTER_FORK_PARENT`` means the function is called from the parent - process after forking a child process. - * ``os.AFTER_FORK_CHILD`` means the function is called from the child - process if control is expected to return to the Python interpreter. + * *before* is a function called before forking a child process. + * *after_in_parent* is a function called from the parent process + after forking a child process. + * *after_in_child* is a function called from the child process + if control is expected to return to the Python interpreter. A + typical :mod:`subprocess` launch will not trigger this as the + child is not going to return to the interpreter. Functions registered for execution before forking are called in reverse registration order. Functions registered for execution @@ -3302,6 +3304,8 @@ written in Python, such as a mail server's external command delivery program. call those functions, unless it explicitly calls :c:func:`PyOS_BeforeFork`, :c:func:`PyOS_AfterFork_Parent` and :c:func:`PyOS_AfterFork_Child`. + There is no way to unregister a function. + Availability: Unix. .. versionadded:: 3.7 diff --git a/Lib/os.py b/Lib/os.py index 47c6e97561548ba..9e7a67feb778e20 100644 --- a/Lib/os.py +++ b/Lib/os.py @@ -94,12 +94,6 @@ def _get_exports_list(module): del _names -if _exists("register_at_fork"): - # Constants for use with os.register_at_fork()'s when parameter. - BEFORE_FORK = "before" - AFTER_FORK_CHILD = "child" - AFTER_FORK_PARENT = "parent" - if _exists("_have_functions"): _globals = globals() def _add(str, fn): diff --git a/Lib/random.py b/Lib/random.py index 4188afd9042bed5..b54d52448820140 100644 --- a/Lib/random.py +++ b/Lib/random.py @@ -765,7 +765,7 @@ def _test(N=2000): getrandbits = _inst.getrandbits if hasattr(_os, "fork"): - _os.register_at_fork(_inst.seed, when=_os.AFTER_FORK_CHILD) + _os.register_at_fork(after_in_child=_inst.seed) if __name__ == '__main__': diff --git a/Lib/test/test_posix.py b/Lib/test/test_posix.py index 596fd903e172616..3c44cf1cce6c54e 100644 --- a/Lib/test/test_posix.py +++ b/Lib/test/test_posix.py @@ -190,18 +190,28 @@ def test_waitid(self): @unittest.skipUnless(hasattr(os, 'fork'), "test needs os.fork()") def test_register_at_fork(self): + with self.assertRaises(TypeError, msg="Positional args not allowed"): + os.register_at_fork(lambda: None) + with self.assertRaises(TypeError, msg="Args must be callable"): + os.register_at_fork(before=2, after_in_child="three", + after_in_parent=b"Five") + with self.assertRaises(TypeError, msg="Args must not be None"): + os.register_at_fork(before=None, after_in_child=None, + after_in_parent=None) + # We test actual registrations in their own process so as not to + # pollute this one. There is no way to unregister for cleanup. code = """if 1: import os r, w = os.pipe() fin_r, fin_w = os.pipe() - os.register_at_fork(lambda: os.write(w, b'A'), when=os.BEFORE_FORK) - os.register_at_fork(lambda: os.write(w, b'B'), when=os.BEFORE_FORK) - os.register_at_fork(lambda: os.write(w, b'C'), when=os.AFTER_FORK_PARENT) - os.register_at_fork(lambda: os.write(w, b'D'), when=os.AFTER_FORK_PARENT) - os.register_at_fork(lambda: os.write(w, b'E'), when=os.AFTER_FORK_CHILD) - os.register_at_fork(lambda: os.write(w, b'F'), when=os.AFTER_FORK_CHILD) + os.register_at_fork(before=lambda: os.write(w, b'A')) + os.register_at_fork(after_in_parent=lambda: os.write(w, b'C')) + os.register_at_fork(after_in_child=lambda: os.write(w, b'E')) + os.register_at_fork(before=lambda: os.write(w, b'B'), + after_in_parent=lambda: os.write(w, b'D'), + after_in_child=lambda: os.write(w, b'F')) pid = os.fork() if pid == 0: diff --git a/Modules/clinic/posixmodule.c.h b/Modules/clinic/posixmodule.c.h index 62204d2267dc674..b5668eee1912ec7 100644 --- a/Modules/clinic/posixmodule.c.h +++ b/Modules/clinic/posixmodule.c.h @@ -1828,40 +1828,45 @@ os_spawnve(PyObject *module, PyObject **args, Py_ssize_t nargs, PyObject *kwname #if defined(HAVE_FORK) PyDoc_STRVAR(os_register_at_fork__doc__, -"register_at_fork($module, func, /, when)\n" +"register_at_fork($module, /, *, before=None, after_in_child=None,\n" +" after_in_parent=None)\n" "--\n" "\n" -"Register a callable object to be called when forking.\n" +"Register a code to be called when forking a new process via os.fork().\n" "\n" -" func\n" -" Function or callable\n" -" when\n" -" os.BEFORE_FORK, os.AFTER_FORK_CHILD, os.AFTER_FORK_PARENT\n" +" before\n" +" Zero arg callable to be called in the parent before the fork() syscall.\n" +" after_in_child\n" +" Zero arg callable to be called in the child before os.fork() returns.\n" +" after_in_parent\n" +" Zero arg callable to be called in the parent before os.fork() returns.\n" "\n" -"os.BEFORE_FORK callbacks are called in reverse order before forking.\n" -"os.AFTER_FORK_CHILD callbacks are called in order after forking, in the child process.\n" -"os.AFTER_FORK_PARENT callbacks are called in order after forking, in the parent process."); +"``before`` callbacks are called in reverse order before forking.\n" +"``after_in_child`` callbacks are called in order after forking in the child.\n" +"``after_in_parent`` callbacks are called in order after forking in the parent."); #define OS_REGISTER_AT_FORK_METHODDEF \ {"register_at_fork", (PyCFunction)os_register_at_fork, METH_FASTCALL, os_register_at_fork__doc__}, static PyObject * -os_register_at_fork_impl(PyObject *module, PyObject *func, const char *when); +os_register_at_fork_impl(PyObject *module, PyObject *before, + PyObject *after_in_child, PyObject *after_in_parent); static PyObject * os_register_at_fork(PyObject *module, PyObject **args, Py_ssize_t nargs, PyObject *kwnames) { PyObject *return_value = NULL; - static const char * const _keywords[] = {"", "when", NULL}; - static _PyArg_Parser _parser = {"Os:register_at_fork", _keywords, 0}; - PyObject *func; - const char *when; + static const char * const _keywords[] = {"before", "after_in_child", "after_in_parent", NULL}; + static _PyArg_Parser _parser = {"|$OOO:register_at_fork", _keywords, 0}; + PyObject *before = NULL; + PyObject *after_in_child = NULL; + PyObject *after_in_parent = NULL; if (!_PyArg_ParseStackAndKeywords(args, nargs, kwnames, &_parser, - &func, &when)) { + &before, &after_in_child, &after_in_parent)) { goto exit; } - return_value = os_register_at_fork_impl(module, func, when); + return_value = os_register_at_fork_impl(module, before, after_in_child, after_in_parent); exit: return return_value; @@ -6541,4 +6546,4 @@ os_getrandom(PyObject *module, PyObject **args, Py_ssize_t nargs, PyObject *kwna #ifndef OS_GETRANDOM_METHODDEF #define OS_GETRANDOM_METHODDEF #endif /* !defined(OS_GETRANDOM_METHODDEF) */ -/*[clinic end generated code: output=30f4881161078e93 input=a9049054013a1b77]*/ +/*[clinic end generated code: output=27bab718edec64c6 input=a9049054013a1b77]*/ diff --git a/Modules/posixmodule.c b/Modules/posixmodule.c index 2c3ecb5498bc315..7c62f7ce529daa5 100644 --- a/Modules/posixmodule.c +++ b/Modules/posixmodule.c @@ -5312,49 +5312,50 @@ os_spawnve_impl(PyObject *module, int mode, path_t *path, PyObject *argv, /*[clinic input] os.register_at_fork - func: object - Function or callable - / - when: str - os.BEFORE_FORK, os.AFTER_FORK_CHILD, os.AFTER_FORK_PARENT + * + before: object=NULL + Zero arg callable to be called in the parent before the fork() syscall. + after_in_child: object=NULL + Zero arg callable to be called in the child before os.fork() returns. + after_in_parent: object=NULL + Zero arg callable to be called in the parent before os.fork() returns. -Register a callable object to be called when forking. +Register a code to be called when forking a new process via os.fork(). -os.BEFORE_FORK callbacks are called in reverse order before forking. -os.AFTER_FORK_CHILD callbacks are called in order after forking, in the child process. -os.AFTER_FORK_PARENT callbacks are called in order after forking, in the parent process. +``before`` callbacks are called in reverse order before forking. +``after_in_child`` callbacks are called in order after forking in the child. +``after_in_parent`` callbacks are called in order after forking in the parent. [clinic start generated code]*/ static PyObject * -os_register_at_fork_impl(PyObject *module, PyObject *func, const char *when) -/*[clinic end generated code: output=8943be81a644750c input=fde1c6bde63beb91]*/ +os_register_at_fork_impl(PyObject *module, PyObject *before, + PyObject *after_in_child, PyObject *after_in_parent) +/*[clinic end generated code: output=5398ac75e8e97625 input=19db3f3cb0d7017a]*/ { PyInterpreterState *interp; - PyObject **lst; - if (!PyCallable_Check(func)) { - PyErr_Format(PyExc_TypeError, - "expected callable object, got %R", Py_TYPE(func)); + if (!(PyCallable_Check(before) || PyCallable_Check(after_in_child) || + PyCallable_Check(after_in_parent))) { + PyErr_SetString(PyExc_TypeError, + "At least one callable argument must be supplied."); return NULL; } interp = PyThreadState_Get()->interp; - if (!strcmp(when, "before")) - lst = &interp->before_forkers; - else if (!strcmp(when, "child")) - lst = &interp->after_forkers_child; - else if (!strcmp(when, "parent")) - lst = &interp->after_forkers_parent; - else { - PyErr_Format(PyExc_ValueError, "unexpected value for `when`: '%s'", - when); + if (before && + register_at_forker(&interp->before_forkers, before)) { return NULL; } - if (register_at_forker(lst, func)) + if (after_in_child && + register_at_forker(&interp->after_forkers_child, after_in_child)) { return NULL; - else - Py_RETURN_NONE; + } + if (after_in_parent && + register_at_forker(&interp->after_forkers_parent, after_in_parent)) { + return NULL; + } + Py_RETURN_NONE; } #endif /* HAVE_FORK */ From 50506fc70bb7522b0ca2a14ba94387358fc5ee6a Mon Sep 17 00:00:00 2001 From: "Gregory P. Smith" Date: Sat, 27 May 2017 16:20:05 -0700 Subject: [PATCH 03/14] Undo the unnecessary blank line os.py change. --- Lib/os.py | 1 + 1 file changed, 1 insertion(+) diff --git a/Lib/os.py b/Lib/os.py index 9e7a67feb778e20..e293ecae7fd3a4e 100644 --- a/Lib/os.py +++ b/Lib/os.py @@ -94,6 +94,7 @@ def _get_exports_list(module): del _names + if _exists("_have_functions"): _globals = globals() def _add(str, fn): From 389b76711b6001585a16e3d9163ea0269d2f3726 Mon Sep 17 00:00:00 2001 From: "Gregory P. Smith" Date: Sat, 27 May 2017 16:22:57 -0700 Subject: [PATCH 04/14] Fix a docstring typo in posixmodule. --- Modules/clinic/posixmodule.c.h | 4 ++-- Modules/posixmodule.c | 4 ++-- 2 files changed, 4 insertions(+), 4 deletions(-) diff --git a/Modules/clinic/posixmodule.c.h b/Modules/clinic/posixmodule.c.h index b5668eee1912ec7..b9e18950996bc64 100644 --- a/Modules/clinic/posixmodule.c.h +++ b/Modules/clinic/posixmodule.c.h @@ -1832,7 +1832,7 @@ PyDoc_STRVAR(os_register_at_fork__doc__, " after_in_parent=None)\n" "--\n" "\n" -"Register a code to be called when forking a new process via os.fork().\n" +"Register a callable to be called when forking a new process via os.fork().\n" "\n" " before\n" " Zero arg callable to be called in the parent before the fork() syscall.\n" @@ -6546,4 +6546,4 @@ os_getrandom(PyObject *module, PyObject **args, Py_ssize_t nargs, PyObject *kwna #ifndef OS_GETRANDOM_METHODDEF #define OS_GETRANDOM_METHODDEF #endif /* !defined(OS_GETRANDOM_METHODDEF) */ -/*[clinic end generated code: output=27bab718edec64c6 input=a9049054013a1b77]*/ +/*[clinic end generated code: output=19831e9abadaac28 input=a9049054013a1b77]*/ diff --git a/Modules/posixmodule.c b/Modules/posixmodule.c index 7c62f7ce529daa5..79bc942c15cbe8e 100644 --- a/Modules/posixmodule.c +++ b/Modules/posixmodule.c @@ -5320,7 +5320,7 @@ os.register_at_fork after_in_parent: object=NULL Zero arg callable to be called in the parent before os.fork() returns. -Register a code to be called when forking a new process via os.fork(). +Register a callable to be called when forking a new process via os.fork(). ``before`` callbacks are called in reverse order before forking. ``after_in_child`` callbacks are called in order after forking in the child. @@ -5331,7 +5331,7 @@ Register a code to be called when forking a new process via os.fork(). static PyObject * os_register_at_fork_impl(PyObject *module, PyObject *before, PyObject *after_in_child, PyObject *after_in_parent) -/*[clinic end generated code: output=5398ac75e8e97625 input=19db3f3cb0d7017a]*/ +/*[clinic end generated code: output=5398ac75e8e97625 input=d69dbd16e6e48e66]*/ { PyInterpreterState *interp; From 36525cee483adb67c99ae98ba26c2890d5967c5d Mon Sep 17 00:00:00 2001 From: "Gregory P. Smith" Date: Sun, 28 May 2017 11:20:04 -0700 Subject: [PATCH 05/14] Don't mention the HAVE_FORK cpp #define. --- Doc/c-api/sys.rst | 3 +-- 1 file changed, 1 insertion(+), 2 deletions(-) diff --git a/Doc/c-api/sys.rst b/Doc/c-api/sys.rst index ac548b8729d29c4..95d9d657ce95e0b 100644 --- a/Doc/c-api/sys.rst +++ b/Doc/c-api/sys.rst @@ -53,8 +53,7 @@ Operating System Utilities This must be called from the child process after calling :c:func:`fork`, or any similar function that clones the current process, if there is any chance the process will call back into the Python interpreter. - Only available on systems where :c:func:`fork` is defined as determined - by the ``HAVE_FORK`` C preprocessor define. + Only available on systems where :c:func:`fork` is defined. .. versionadded:: 3.7 From 6ce5170a75dce6acd8c822bb73be0bcefb71c6f9 Mon Sep 17 00:00:00 2001 From: "Gregory P. Smith" Date: Sun, 28 May 2017 11:21:54 -0700 Subject: [PATCH 06/14] Don't use backticks in the docstring. --- Modules/clinic/posixmodule.c.h | 8 ++++---- Modules/posixmodule.c | 8 ++++---- 2 files changed, 8 insertions(+), 8 deletions(-) diff --git a/Modules/clinic/posixmodule.c.h b/Modules/clinic/posixmodule.c.h index b9e18950996bc64..2f12582dd3881af 100644 --- a/Modules/clinic/posixmodule.c.h +++ b/Modules/clinic/posixmodule.c.h @@ -1841,9 +1841,9 @@ PyDoc_STRVAR(os_register_at_fork__doc__, " after_in_parent\n" " Zero arg callable to be called in the parent before os.fork() returns.\n" "\n" -"``before`` callbacks are called in reverse order before forking.\n" -"``after_in_child`` callbacks are called in order after forking in the child.\n" -"``after_in_parent`` callbacks are called in order after forking in the parent."); +"\'before\' callbacks are called in reverse order before forking.\n" +"\'after_in_child\' callbacks are called in order after forking in the child.\n" +"\'after_in_parent\' callbacks are called in order after forking in the parent."); #define OS_REGISTER_AT_FORK_METHODDEF \ {"register_at_fork", (PyCFunction)os_register_at_fork, METH_FASTCALL, os_register_at_fork__doc__}, @@ -6546,4 +6546,4 @@ os_getrandom(PyObject *module, PyObject **args, Py_ssize_t nargs, PyObject *kwna #ifndef OS_GETRANDOM_METHODDEF #define OS_GETRANDOM_METHODDEF #endif /* !defined(OS_GETRANDOM_METHODDEF) */ -/*[clinic end generated code: output=19831e9abadaac28 input=a9049054013a1b77]*/ +/*[clinic end generated code: output=e87a841007afa62b input=a9049054013a1b77]*/ diff --git a/Modules/posixmodule.c b/Modules/posixmodule.c index 79bc942c15cbe8e..4b154ea0aeea67f 100644 --- a/Modules/posixmodule.c +++ b/Modules/posixmodule.c @@ -5322,16 +5322,16 @@ os.register_at_fork Register a callable to be called when forking a new process via os.fork(). -``before`` callbacks are called in reverse order before forking. -``after_in_child`` callbacks are called in order after forking in the child. -``after_in_parent`` callbacks are called in order after forking in the parent. +'before' callbacks are called in reverse order before forking. +'after_in_child' callbacks are called in order after forking in the child. +'after_in_parent' callbacks are called in order after forking in the parent. [clinic start generated code]*/ static PyObject * os_register_at_fork_impl(PyObject *module, PyObject *before, PyObject *after_in_child, PyObject *after_in_parent) -/*[clinic end generated code: output=5398ac75e8e97625 input=d69dbd16e6e48e66]*/ +/*[clinic end generated code: output=5398ac75e8e97625 input=44962e2b3bb9ce2e]*/ { PyInterpreterState *interp; From 1f07b5cdb6aeb1d777c2f8bba42b30e0d0a961c9 Mon Sep 17 00:00:00 2001 From: "Gregory P. Smith" Date: Sun, 28 May 2017 11:31:37 -0700 Subject: [PATCH 07/14] bpo-16500, don't call forkers if we never forked(). We don't want to call PyOS_BeforeFork() or PyOS_AfterFork_Parent() if we never manage to make it to the fork() call it self, erroring out instead. --- Modules/_posixsubprocess.c | 21 +++++++++++---------- 1 file changed, 11 insertions(+), 10 deletions(-) diff --git a/Modules/_posixsubprocess.c b/Modules/_posixsubprocess.c index 5228fecfa99d923..8c8777cfe33f5bd 100644 --- a/Modules/_posixsubprocess.c +++ b/Modules/_posixsubprocess.c @@ -651,14 +651,6 @@ subprocess_fork_exec(PyObject* self, PyObject *args) goto cleanup; } - if (preexec_fn != Py_None) { - preexec_fn_args_tuple = PyTuple_New(0); - if (!preexec_fn_args_tuple) - goto cleanup; - PyOS_BeforeFork(); - need_after_fork = 1; - } - if (cwd_obj != Py_None) { if (PyUnicode_FSConverter(cwd_obj, &cwd_obj2) == 0) goto cleanup; @@ -668,6 +660,17 @@ subprocess_fork_exec(PyObject* self, PyObject *args) cwd_obj2 = NULL; } + /* This must be the last thing done before fork() because we do not + * want to call PyOS_BeforeFork() if there is any chance of another + * error leading to the cleanup: code without calling fork(). */ + if (preexec_fn != Py_None) { + preexec_fn_args_tuple = PyTuple_New(0); + if (!preexec_fn_args_tuple) + goto cleanup; + PyOS_BeforeFork(); + need_after_fork = 1; + } + pid = fork(); if (pid == 0) { /* Child process */ @@ -722,8 +725,6 @@ subprocess_fork_exec(PyObject* self, PyObject *args) return PyLong_FromPid(pid); cleanup: - if (need_after_fork) - PyOS_AfterFork_Parent(); if (envp) _Py_FreeCharPArray(envp); if (argv) From c51902bef300f48c0fb353cd07471d861e5ba4e4 Mon Sep 17 00:00:00 2001 From: "Gregory P. Smith" Date: Sun, 28 May 2017 11:35:57 -0700 Subject: [PATCH 08/14] bpo-16500: Clarify when all three calls are made. The statement about re-entering the interpreter applies to all three calls, not just after_in_child. --- Doc/library/os.rst | 9 +++++---- 1 file changed, 5 insertions(+), 4 deletions(-) diff --git a/Doc/library/os.rst b/Doc/library/os.rst index 33899939b480a37..64b61a9ca9b639c 100644 --- a/Doc/library/os.rst +++ b/Doc/library/os.rst @@ -3290,10 +3290,11 @@ written in Python, such as a mail server's external command delivery program. * *before* is a function called before forking a child process. * *after_in_parent* is a function called from the parent process after forking a child process. - * *after_in_child* is a function called from the child process - if control is expected to return to the Python interpreter. A - typical :mod:`subprocess` launch will not trigger this as the - child is not going to return to the interpreter. + * *after_in_child* is a function called from the child process. + + These calls are only made if control is expected to return to the + Python interpreter. A typical :mod:`subprocess` launch will not + trigger them as the child is not going to re-enter the interpreter. Functions registered for execution before forking are called in reverse registration order. Functions registered for execution From 3ee495d89f476ba02e459f779425079708164357 Mon Sep 17 00:00:00 2001 From: "Gregory P. Smith" Date: Sun, 28 May 2017 12:11:20 -0700 Subject: [PATCH 09/14] Improve the test cases to cover the required logic. --- Lib/test/test_posix.py | 20 ++++++++++++++++---- 1 file changed, 16 insertions(+), 4 deletions(-) diff --git a/Lib/test/test_posix.py b/Lib/test/test_posix.py index 3c44cf1cce6c54e..412b0799431c746 100644 --- a/Lib/test/test_posix.py +++ b/Lib/test/test_posix.py @@ -193,11 +193,23 @@ def test_register_at_fork(self): with self.assertRaises(TypeError, msg="Positional args not allowed"): os.register_at_fork(lambda: None) with self.assertRaises(TypeError, msg="Args must be callable"): - os.register_at_fork(before=2, after_in_child="three", - after_in_parent=b"Five") + os.register_at_fork(before=2) + with self.assertRaises(TypeError, msg="Args must be callable"): + os.register_at_fork(after_in_child="three") + with self.assertRaises(TypeError, msg="Args must be callable"): + os.register_at_fork(after_in_parent=b"Five") + with self.assertRaises(TypeError, msg="Args must not be None"): + os.register_at_fork(before=None) + with self.assertRaises(TypeError, msg="Args must not be None"): + os.register_at_fork(after_in_child=None) with self.assertRaises(TypeError, msg="Args must not be None"): - os.register_at_fork(before=None, after_in_child=None, - after_in_parent=None) + os.register_at_fork(after_in_parent=None) + with self.assertRaises(TypeError, msg="Invalid arg was allowed"): + # Ensure a combination of valid and invalid is an error. + os.register_at_fork(before=None, after_in_parent=lambda: 3) + with self.assertRaises(TypeError, msg="Invalid arg was allowed"): + # Ensure a combination of valid and invalid is an error. + os.register_at_fork(before=lambda: None, after_in_child='') # We test actual registrations in their own process so as not to # pollute this one. There is no way to unregister for cleanup. code = """if 1: From 539c285b8e1334dd8b84ae880f0ea66fb0cb7d73 Mon Sep 17 00:00:00 2001 From: "Gregory P. Smith" Date: Sun, 28 May 2017 12:38:19 -0700 Subject: [PATCH 10/14] fix register_at_fork parameter validation. --- Modules/posixmodule.c | 37 +++++++++++++++++++++++++++---------- 1 file changed, 27 insertions(+), 10 deletions(-) diff --git a/Modules/posixmodule.c b/Modules/posixmodule.c index 4b154ea0aeea67f..cd979b175c5749e 100644 --- a/Modules/posixmodule.c +++ b/Modules/posixmodule.c @@ -465,6 +465,8 @@ PyOS_AfterFork_Child(void) static int register_at_forker(PyObject **lst, PyObject *func) { + if (func == NULL) /* nothing to register? do nothing. */ + return 0; if (*lst == NULL) { *lst = PyList_New(0); if (*lst == NULL) @@ -5309,6 +5311,21 @@ os_spawnve_impl(PyObject *module, int mode, path_t *path, PyObject *argv, #ifdef HAVE_FORK + +/* Helper function to validate arguments. + Returns 0 on success. 1 on failure with a TypeError raised. + If obj is non-NULL it must be callable. */ +static int +check_null_or_callable(PyObject *obj, const char* obj_name) +{ + if (obj && !PyCallable_Check(obj)) { + PyErr_Format(PyExc_TypeError, "'%s' must be callable, not %R", + obj_name, Py_TYPE(obj)); + return 1; + } + return 0; +} + /*[clinic input] os.register_at_fork @@ -5335,24 +5352,24 @@ os_register_at_fork_impl(PyObject *module, PyObject *before, { PyInterpreterState *interp; - if (!(PyCallable_Check(before) || PyCallable_Check(after_in_child) || - PyCallable_Check(after_in_parent))) { - PyErr_SetString(PyExc_TypeError, - "At least one callable argument must be supplied."); + if (!before && !after_in_child && !after_in_parent) { + PyErr_SetString(PyExc_TypeError, "At least one argument is required."); + return NULL; + } + if (check_null_or_callable(before, "before") || + check_null_or_callable(after_in_child, "after_in_child") || + check_null_or_callable(after_in_parent, "after_in_parent")) { return NULL; } interp = PyThreadState_Get()->interp; - if (before && - register_at_forker(&interp->before_forkers, before)) { + if (register_at_forker(&interp->before_forkers, before)) { return NULL; } - if (after_in_child && - register_at_forker(&interp->after_forkers_child, after_in_child)) { + if (register_at_forker(&interp->after_forkers_child, after_in_child)) { return NULL; } - if (after_in_parent && - register_at_forker(&interp->after_forkers_parent, after_in_parent)) { + if (register_at_forker(&interp->after_forkers_parent, after_in_parent)) { return NULL; } Py_RETURN_NONE; From 81597a6d9f079fd3686ad08962e56c433eaee8ef Mon Sep 17 00:00:00 2001 From: "Gregory P. Smith" Date: Sun, 28 May 2017 13:22:59 -0700 Subject: [PATCH 11/14] update threading to use the new API. --- Lib/threading.py | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/Lib/threading.py b/Lib/threading.py index 2eaf49a611f658b..92c2ab365b4f912 100644 --- a/Lib/threading.py +++ b/Lib/threading.py @@ -1359,5 +1359,5 @@ def _after_fork(): assert len(_active) == 1 -if hasattr(_os, "fork"): - _os.register_at_fork(_after_fork, when="child") +if hasattr(_os, "register_at_fork"): + _os.register_at_fork(after_in_child=_after_fork) From 52db2afc4c5281ca8fbbc510edb689abda8268ef Mon Sep 17 00:00:00 2001 From: "Gregory P. Smith" Date: Sun, 28 May 2017 13:49:49 -0700 Subject: [PATCH 12/14] Documentation refactoring based on code review. --- Doc/library/os.rst | 3 ++- Modules/clinic/posixmodule.c.h | 15 +++++++-------- Modules/posixmodule.c | 19 +++++++++---------- 3 files changed, 18 insertions(+), 19 deletions(-) diff --git a/Doc/library/os.rst b/Doc/library/os.rst index 64b61a9ca9b639c..86add0cb8e17dcb 100644 --- a/Doc/library/os.rst +++ b/Doc/library/os.rst @@ -3284,7 +3284,8 @@ written in Python, such as a mail server's external command delivery program. after_in_child=None) Register callables to be executed when a new child process is forked - using :func:`os.fork`. The parameters are optional and keyword-only. + using :func:`os.fork` or similar process cloning APIs. + The parameters are optional and keyword-only. Each specifies a different call point. * *before* is a function called before forking a child process. diff --git a/Modules/clinic/posixmodule.c.h b/Modules/clinic/posixmodule.c.h index 2f12582dd3881af..831b5e9d2716549 100644 --- a/Modules/clinic/posixmodule.c.h +++ b/Modules/clinic/posixmodule.c.h @@ -1832,18 +1832,17 @@ PyDoc_STRVAR(os_register_at_fork__doc__, " after_in_parent=None)\n" "--\n" "\n" -"Register a callable to be called when forking a new process via os.fork().\n" +"Registers callables to be called when forking a new process.\n" "\n" " before\n" -" Zero arg callable to be called in the parent before the fork() syscall.\n" +" A callable to be called in the parent before the fork() syscall.\n" " after_in_child\n" -" Zero arg callable to be called in the child before os.fork() returns.\n" +" A callable to be called in the child after fork().\n" " after_in_parent\n" -" Zero arg callable to be called in the parent before os.fork() returns.\n" +" A callable to be called in the parent after fork().\n" "\n" -"\'before\' callbacks are called in reverse order before forking.\n" -"\'after_in_child\' callbacks are called in order after forking in the child.\n" -"\'after_in_parent\' callbacks are called in order after forking in the parent."); +"\'before\' callbacks are called in reverse order.\n" +"\'after_in_child\' and \'after_in_parent\' callbacks are called in order."); #define OS_REGISTER_AT_FORK_METHODDEF \ {"register_at_fork", (PyCFunction)os_register_at_fork, METH_FASTCALL, os_register_at_fork__doc__}, @@ -6546,4 +6545,4 @@ os_getrandom(PyObject *module, PyObject **args, Py_ssize_t nargs, PyObject *kwna #ifndef OS_GETRANDOM_METHODDEF #define OS_GETRANDOM_METHODDEF #endif /* !defined(OS_GETRANDOM_METHODDEF) */ -/*[clinic end generated code: output=e87a841007afa62b input=a9049054013a1b77]*/ +/*[clinic end generated code: output=1c548b06f18b8358 input=a9049054013a1b77]*/ diff --git a/Modules/posixmodule.c b/Modules/posixmodule.c index cd979b175c5749e..7322b92f60e363a 100644 --- a/Modules/posixmodule.c +++ b/Modules/posixmodule.c @@ -5313,7 +5313,7 @@ os_spawnve_impl(PyObject *module, int mode, path_t *path, PyObject *argv, #ifdef HAVE_FORK /* Helper function to validate arguments. - Returns 0 on success. 1 on failure with a TypeError raised. + Returns 0 on success. non-zero on failure with a TypeError raised. If obj is non-NULL it must be callable. */ static int check_null_or_callable(PyObject *obj, const char* obj_name) @@ -5321,7 +5321,7 @@ check_null_or_callable(PyObject *obj, const char* obj_name) if (obj && !PyCallable_Check(obj)) { PyErr_Format(PyExc_TypeError, "'%s' must be callable, not %R", obj_name, Py_TYPE(obj)); - return 1; + return -1; } return 0; } @@ -5331,24 +5331,23 @@ os.register_at_fork * before: object=NULL - Zero arg callable to be called in the parent before the fork() syscall. + A callable to be called in the parent before the fork() syscall. after_in_child: object=NULL - Zero arg callable to be called in the child before os.fork() returns. + A callable to be called in the child after fork(). after_in_parent: object=NULL - Zero arg callable to be called in the parent before os.fork() returns. + A callable to be called in the parent after fork(). -Register a callable to be called when forking a new process via os.fork(). +Registers callables to be called when forking a new process. -'before' callbacks are called in reverse order before forking. -'after_in_child' callbacks are called in order after forking in the child. -'after_in_parent' callbacks are called in order after forking in the parent. +'before' callbacks are called in reverse order. +'after_in_child' and 'after_in_parent' callbacks are called in order. [clinic start generated code]*/ static PyObject * os_register_at_fork_impl(PyObject *module, PyObject *before, PyObject *after_in_child, PyObject *after_in_parent) -/*[clinic end generated code: output=5398ac75e8e97625 input=44962e2b3bb9ce2e]*/ +/*[clinic end generated code: output=5398ac75e8e97625 input=78bc3d28ba7bf117]*/ { PyInterpreterState *interp; From 22a40c99be49f7b6ea68e779f9b148bf08c02d22 Mon Sep 17 00:00:00 2001 From: "Gregory P. Smith" Date: Sun, 28 May 2017 13:55:03 -0700 Subject: [PATCH 13/14] Use the type name rather than repr in the error. --- Modules/posixmodule.c | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/Modules/posixmodule.c b/Modules/posixmodule.c index 7322b92f60e363a..bf8b1e49ace98bd 100644 --- a/Modules/posixmodule.c +++ b/Modules/posixmodule.c @@ -5319,8 +5319,8 @@ static int check_null_or_callable(PyObject *obj, const char* obj_name) { if (obj && !PyCallable_Check(obj)) { - PyErr_Format(PyExc_TypeError, "'%s' must be callable, not %R", - obj_name, Py_TYPE(obj)); + PyErr_Format(PyExc_TypeError, "'%s' must be callable, not %s", + obj_name, Py_TYPE(obj)->tp_name); return -1; } return 0; From 51327970a34625cebbf8cf5dd0f943c5c8b0aad6 Mon Sep 17 00:00:00 2001 From: "Gregory P. Smith" Date: Mon, 29 May 2017 09:21:37 -0700 Subject: [PATCH 14/14] Use the infinitive in the docstring. --- Modules/clinic/posixmodule.c.h | 4 ++-- Modules/posixmodule.c | 4 ++-- 2 files changed, 4 insertions(+), 4 deletions(-) diff --git a/Modules/clinic/posixmodule.c.h b/Modules/clinic/posixmodule.c.h index 831b5e9d2716549..8e1b55a57ddc141 100644 --- a/Modules/clinic/posixmodule.c.h +++ b/Modules/clinic/posixmodule.c.h @@ -1832,7 +1832,7 @@ PyDoc_STRVAR(os_register_at_fork__doc__, " after_in_parent=None)\n" "--\n" "\n" -"Registers callables to be called when forking a new process.\n" +"Register callables to be called when forking a new process.\n" "\n" " before\n" " A callable to be called in the parent before the fork() syscall.\n" @@ -6545,4 +6545,4 @@ os_getrandom(PyObject *module, PyObject **args, Py_ssize_t nargs, PyObject *kwna #ifndef OS_GETRANDOM_METHODDEF #define OS_GETRANDOM_METHODDEF #endif /* !defined(OS_GETRANDOM_METHODDEF) */ -/*[clinic end generated code: output=1c548b06f18b8358 input=a9049054013a1b77]*/ +/*[clinic end generated code: output=dce741f527ddbfa4 input=a9049054013a1b77]*/ diff --git a/Modules/posixmodule.c b/Modules/posixmodule.c index bf8b1e49ace98bd..f4a21679d0b2027 100644 --- a/Modules/posixmodule.c +++ b/Modules/posixmodule.c @@ -5337,7 +5337,7 @@ os.register_at_fork after_in_parent: object=NULL A callable to be called in the parent after fork(). -Registers callables to be called when forking a new process. +Register callables to be called when forking a new process. 'before' callbacks are called in reverse order. 'after_in_child' and 'after_in_parent' callbacks are called in order. @@ -5347,7 +5347,7 @@ Registers callables to be called when forking a new process. static PyObject * os_register_at_fork_impl(PyObject *module, PyObject *before, PyObject *after_in_child, PyObject *after_in_parent) -/*[clinic end generated code: output=5398ac75e8e97625 input=78bc3d28ba7bf117]*/ +/*[clinic end generated code: output=5398ac75e8e97625 input=cd1187aa85d2312e]*/ { PyInterpreterState *interp;