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.

8 files changed, 53 insertions, 28 deletions

diff --git a/Doc/whatsnew/3.10.rst b/Doc/whatsnew/3.10.rst
index 23e28aa..b690f8d 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 2515234..12416c2 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 b691e63..e6e4187 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 864cea3..0a4372e 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 0000000..bdb2edd
--- /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 f1fc871..90ed7d5 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 54a35a2..7b80d01 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 ca5a04d..e055e44 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']))