From 357704c9f2375f29ed5b3a93adac086fa714538d Mon Sep 17 00:00:00 2001 From: Victor Stinner Date: Mon, 14 Dec 2020 23:07:54 +0100 Subject: [PATCH] bpo-42639: atexit now logs callbacks exceptions (GH-23771) At Python exit, if a callback registered with atexit.register() fails, its exception is now logged. Previously, only some exceptions were logged, and the last exception was always silently ignored. Add _PyAtExit_Call() function and remove PyInterpreterState.atexit_func member. call_py_exitfuncs() now calls directly _PyAtExit_Call(). The atexit module must now always be built as a built-in module. --- Doc/whatsnew/3.10.rst | 9 ++++ Include/internal/pycore_interp.h | 2 +- Include/internal/pycore_pylifecycle.h | 2 + Lib/test/test_threading.py | 1 - .../2020-12-14-22-31-22.bpo-42639.5Z3iWX.rst | 3 ++ Modules/atexitmodule.c | 53 ++++++++++++------- Python/pylifecycle.c | 9 ++-- setup.py | 2 - 8 files changed, 53 insertions(+), 28 deletions(-) create mode 100644 Misc/NEWS.d/next/Library/2020-12-14-22-31-22.bpo-42639.5Z3iWX.rst diff --git a/Doc/whatsnew/3.10.rst b/Doc/whatsnew/3.10.rst index 23e28aa4fd8..b690f8d2d7c 100644 --- a/Doc/whatsnew/3.10.rst +++ b/Doc/whatsnew/3.10.rst @@ -501,6 +501,13 @@ Changes in the Python API have been renamed to *exc*. (Contributed by Zackery Spytz and Matthias Bussonnier in :issue:`26389`.) +* :mod:`atexit`: At Python exit, if a callback registered with + :func:`atexit.register` fails, its exception is now logged. Previously, only + some exceptions were logged, and the last exception was always silently + ignored. + (Contributed by Victor Stinner in :issue:`42639`.) + + CPython bytecode changes ======================== @@ -519,6 +526,8 @@ Build Changes * :mod:`sqlite3` requires SQLite 3.7.3 or higher. (Contributed by Sergey Fedoseev and Erlend E. Aasland :issue:`40744`.) +* The :mod:`atexit` module must now always be built as a built-in module. + (Contributed by Victor Stinner in :issue:`42639`.) C API Changes diff --git a/Include/internal/pycore_interp.h b/Include/internal/pycore_interp.h index 2515234708b..12416c2e94d 100644 --- a/Include/internal/pycore_interp.h +++ b/Include/internal/pycore_interp.h @@ -233,8 +233,8 @@ struct _is { PyObject *after_forkers_parent; PyObject *after_forkers_child; #endif + /* AtExit module */ - void (*atexit_func)(PyObject *); PyObject *atexit_module; uint64_t tstate_next_unique_id; diff --git a/Include/internal/pycore_pylifecycle.h b/Include/internal/pycore_pylifecycle.h index b691e632578..e6e4187cd5a 100644 --- a/Include/internal/pycore_pylifecycle.h +++ b/Include/internal/pycore_pylifecycle.h @@ -109,6 +109,8 @@ PyAPI_FUNC(void) _PyErr_Display(PyObject *file, PyObject *exception, PyAPI_FUNC(void) _PyThreadState_DeleteCurrent(PyThreadState *tstate); +extern void _PyAtExit_Call(PyObject *module); + #ifdef __cplusplus } #endif diff --git a/Lib/test/test_threading.py b/Lib/test/test_threading.py index 864cea313ae..0a4372ec2df 100644 --- a/Lib/test/test_threading.py +++ b/Lib/test/test_threading.py @@ -487,7 +487,6 @@ class ThreadTests(BaseTestCase): if not pid: print("child process ok", file=sys.stderr, flush=True) # child process - sys.exit() else: wait_process(pid, exitcode=0) diff --git a/Misc/NEWS.d/next/Library/2020-12-14-22-31-22.bpo-42639.5Z3iWX.rst b/Misc/NEWS.d/next/Library/2020-12-14-22-31-22.bpo-42639.5Z3iWX.rst new file mode 100644 index 00000000000..bdb2edd7adc --- /dev/null +++ b/Misc/NEWS.d/next/Library/2020-12-14-22-31-22.bpo-42639.5Z3iWX.rst @@ -0,0 +1,3 @@ +At Python exit, if a callback registered with :func:`atexit.register` fails, +its exception is now logged. Previously, only some exceptions were logged, and +the last exception was always silently ignored. diff --git a/Modules/atexitmodule.c b/Modules/atexitmodule.c index f1fc871cf7f..90ed7d50cad 100644 --- a/Modules/atexitmodule.c +++ b/Modules/atexitmodule.c @@ -65,7 +65,7 @@ atexit_cleanup(struct atexit_state *state) /* Installed into pylifecycle.c's atexit mechanism */ static void -atexit_callfuncs(PyObject *module) +atexit_callfuncs(PyObject *module, int ignore_exc) { assert(!PyErr_Occurred()); @@ -87,18 +87,23 @@ atexit_callfuncs(PyObject *module) PyObject *res = PyObject_Call(cb->func, cb->args, cb->kwargs); if (res == NULL) { - /* Maintain the last exception, but don't leak if there are - multiple exceptions. */ - if (exc_type) { - Py_DECREF(exc_type); - Py_XDECREF(exc_value); - Py_XDECREF(exc_tb); + if (ignore_exc) { + _PyErr_WriteUnraisableMsg("in atexit callback", cb->func); } - PyErr_Fetch(&exc_type, &exc_value, &exc_tb); - if (!PyErr_GivenExceptionMatches(exc_type, PyExc_SystemExit)) { - PySys_WriteStderr("Error in atexit._run_exitfuncs:\n"); - PyErr_NormalizeException(&exc_type, &exc_value, &exc_tb); - PyErr_Display(exc_type, exc_value, exc_tb); + else { + /* Maintain the last exception, but don't leak if there are + multiple exceptions. */ + if (exc_type) { + Py_DECREF(exc_type); + Py_XDECREF(exc_value); + Py_XDECREF(exc_tb); + } + PyErr_Fetch(&exc_type, &exc_value, &exc_tb); + if (!PyErr_GivenExceptionMatches(exc_type, PyExc_SystemExit)) { + PySys_WriteStderr("Error in atexit._run_exitfuncs:\n"); + PyErr_NormalizeException(&exc_type, &exc_value, &exc_tb); + PyErr_Display(exc_type, exc_value, exc_tb); + } } } else { @@ -108,11 +113,24 @@ atexit_callfuncs(PyObject *module) atexit_cleanup(state); - if (exc_type) { - PyErr_Restore(exc_type, exc_value, exc_tb); + if (ignore_exc) { + assert(!PyErr_Occurred()); + } + else { + if (exc_type) { + PyErr_Restore(exc_type, exc_value, exc_tb); + } } } + +void +_PyAtExit_Call(PyObject *module) +{ + atexit_callfuncs(module, 1); +} + + /* ===================================================================== */ /* Module methods. */ @@ -180,7 +198,7 @@ Run all registered exit functions."); static PyObject * atexit_run_exitfuncs(PyObject *module, PyObject *unused) { - atexit_callfuncs(module); + atexit_callfuncs(module, 0); if (PyErr_Occurred()) { return NULL; } @@ -308,9 +326,8 @@ atexit_exec(PyObject *module) return -1; } - PyInterpreterState *is = _PyInterpreterState_GET(); - is->atexit_func = atexit_callfuncs; - is->atexit_module = module; + PyInterpreterState *interp = _PyInterpreterState_GET(); + interp->atexit_module = module; return 0; } diff --git a/Python/pylifecycle.c b/Python/pylifecycle.c index 54a35a27ecc..7b80d01a375 100644 --- a/Python/pylifecycle.c +++ b/Python/pylifecycle.c @@ -2632,19 +2632,16 @@ Py_ExitStatusException(PyStatus status) } } + /* Clean up and exit */ static void call_py_exitfuncs(PyThreadState *tstate) { - PyInterpreterState *interp = tstate->interp; - if (interp->atexit_func == NULL) - return; - - interp->atexit_func(interp->atexit_module); - _PyErr_Clear(tstate); + _PyAtExit_Call(tstate->interp->atexit_module); } + /* Wait until threading._shutdown completes, provided the threading module was imported in the first place. The shutdown routine will wait until all non-daemon diff --git a/setup.py b/setup.py index ca5a04d2ae0..e055e44b0f1 100644 --- a/setup.py +++ b/setup.py @@ -854,8 +854,6 @@ class PyBuildExt(build_ext): # C-optimized pickle replacement self.add(Extension("_pickle", ["_pickle.c"], extra_compile_args=['-DPy_BUILD_CORE_MODULE'])) - # atexit - self.add(Extension("atexit", ["atexitmodule.c"])) # _json speedups self.add(Extension("_json", ["_json.c"], extra_compile_args=['-DPy_BUILD_CORE_MODULE']))