python · gpshead · Mar 19, 2024 · Mar 19, 2024 · gpshead · Mar 19, 2024
diff --git a/Doc/library/atexit.rst b/Doc/library/atexit.rst
@@ -48,17 +48,22 @@ a cleanup function is undefined.
    This function returns *func*, which makes it possible to use it as a
    decorator.
 
-   .. warning::
-       Starting new threads or calling :func:`os.fork` from a registered
-       function can lead to race condition between the main Python
-       runtime thread freeing thread states while internal :mod:`threading`
-       routines or the new process try to use that state. This can lead to
-       crashes rather than clean shutdown.
+   .. note::
+       Calling :func:`os.fork` from a registered function can lead to race
+       condition between the main Python runtime thread freeing thread states
+       while internal :mod:`threading` routines or the new process try to use
+       that state. This can lead to crashes rather than clean shutdown. In
+       Python versions prior to 3.12 starting a new thread from a registered
+       atexit handler function could lead to similar problems.
 
    .. versionchanged:: 3.12
        Attempts to start a new thread or :func:`os.fork` a new process
        in a registered function now leads to :exc:`RuntimeError`.
 
+   .. versionchanged:: 3.13
+       The ability to start a new thread from a registered function has been
+       restored.
+
 .. function:: unregister(func)
 
    Remove *func* from the list of functions to be run at interpreter shutdown.

@@ -52,6 +52,10 @@ struct atexit_state {
     atexit_py_callback **callbacks;
     int ncallbacks;
     int callback_len;
+
+    // Used to prevent registering of new atexit functions once atexit
+    // processing has started.  Boolean, use atomic access.
+    int handler_calling_started;
 };
 
 // Export for '_xxinterpchannels' shared extension

@@ -102,7 +102,7 @@ struct _is {
        In order to be effective, this must be set to 0 during or right
        after allocation. */
     int _initialized;
-    int finalizing;
+    int started_finalization;  /* set early, used to block some actions. */
 
     uintptr_t last_restart_version;
     struct pythreads {
@@ -125,7 +125,7 @@ struct _is {
        Get runtime from tstate: tstate->interp->runtime. */
     struct pyruntimestate *runtime;
 
-    /* Set by Py_EndInterpreter().
+    /* Set by Py_EndInterpreter() & Py_FinalizeEx().
 
        Use _PyInterpreterState_GetFinalizing()
        and _PyInterpreterState_SetFinalizing()

@@ -329,6 +329,9 @@ def _bootstrap(self, parent_sentinel=None):
             sys.stderr.write('Process %s:\n' % self.name)
             traceback.print_exc()
         finally:
+            # bpo-18966: Explicitly Join threads: .popen_fork.Popen.launch()
+            # and .forkserver._main() both call os._exit() after this which
+            # skips that.
             threading._shutdown()
             util.info('process exiting with exitcode %d' % exitcode)
             util._flush_std_streams()

diff --git a/Lib/test/test_threading.py b/Lib/test/test_threading.py
@@ -1155,21 +1155,40 @@ def import_threading():
         self.assertEqual(err, b'')
 
     def test_start_new_thread_at_exit(self):
+        # The low level private API version of gh-113964.
         code = """if 1:
             import atexit
             import _thread
 
             def f():
-                print("shouldn't be printed")
+                pass
 
             def exit_handler():
                 _thread.start_new_thread(f, ())
 
             atexit.register(exit_handler)
         """
         _, out, err = assert_python_ok("-c", code)
-        self.assertEqual(out, b'')
-        self.assertIn(b"can't create new thread at interpreter shutdown", err)
+        self.assertEqual(err.strip(), b'')
+
+    def test_start_threading_thread_at_exit(self):
+        # gh-113964: atexit needs to be able to start threads.
+        code = """if 1:
+            import atexit
+            import threading
+
+            def f():
+                print("should also be printed")
+
+            def exit_handler():
+                threading.Thread(target=f).start()
+
+            atexit.register(exit_handler)
+        """
+        _, out, err = assert_python_ok("-c", code)
+        self.assertEqual(out.strip(), b'should also be printed',
+                         msg=err.decode('utf-8', errors='replace'))
+
 
 class ThreadJoinOnShutdown(BaseTestCase):
 

diff --git a/Lib/threading.py b/Lib/threading.py
@@ -1515,22 +1515,19 @@ def _shutdown():
     """
     Wait until the Python thread state of all non-daemon threads get deleted.
     """
-    # Obscure: other threads may be waiting to join _main_thread.  That's
-    # dubious, but some code does it. We can't wait for it to be marked as done
-    # normally - that won't happen until the interpreter is nearly dead. So
-    # mark it done here.
-    if _main_thread._handle.is_done() and _is_main_interpreter():
-        # _shutdown() was already called
-        return
-
     global _SHUTTING_DOWN
     _SHUTTING_DOWN = True
 
     # Call registered threading atexit functions before threads are joined.
     # Order is reversed, similar to atexit.
-    for atexit_call in reversed(_threading_atexits):
+    while _threading_atexits and (atexit_call := _threading_atexits.pop()):
         atexit_call()
 
+    # Obscure: Other threads may be waiting to join _main_thread.  That's
+    # dubious, but some code does it.  They can't wait for it to be marked done
+    # normally - that won't happen until the interpreter is nearly dead.  So
+    # mark it done here so they can proceed and be joined below.
+    # BPO-18984's https://github.com/python/cpython/commit/c363a23eff25d8ee74d
     if _is_main_interpreter():
         _main_thread._handle._set_done()
 

diff --git a/Misc/NEWS.d/next/Core and Builtins/2024-03-19-01-40-25.gh-issue-113964.QzVjUV.rst b/Misc/NEWS.d/next/Core and Builtins/2024-03-19-01-40-25.gh-issue-113964.QzVjUV.rst
@@ -0,0 +1,5 @@
+Restores the ability to spawn threads from :mod:`atexit` handlers. The
+interpreter finalization code will join non-daemon threads spawned from that
+context before exiting.  No atexit handlers can be registered once atexit
+handler processing has started and none will remain to run after threads
+spawned during atexit processing exit.
@@ -1031,7 +1031,7 @@ subprocess_fork_exec_impl(PyObject *module, PyObject *process_args,
     Py_ssize_t fds_to_keep_len = PyTuple_GET_SIZE(py_fds_to_keep);
 
     PyInterpreterState *interp = _PyInterpreterState_GET();
-    if ((preexec_fn != Py_None) && interp->finalizing) {
+    if ((preexec_fn != Py_None) && interp->started_finalization) {
         PyErr_SetString(PyExc_PythonFinalizationError,
                         "preexec_fn not supported at interpreter shutdown");
         return NULL;

diff --git a/Modules/_threadmodule.c b/Modules/_threadmodule.c
@@ -1729,7 +1729,9 @@ do_start_new_thread(thread_module_state *state, PyObject *func, PyObject *args,
                         "thread is not supported for isolated subinterpreters");
         return -1;
     }
-    if (interp->finalizing) {
+    if (_PyInterpreterState_GetFinalizing(interp)) {
+        // interp->started_finalizing is set too early; exit handlers may
+        // have a need to spawn cleanup threads that finalize waits on.
         PyErr_SetString(PyExc_PythonFinalizationError,
                         "can't create new thread at interpreter shutdown");
         return -1;

diff --git a/Modules/atexitmodule.c b/Modules/atexitmodule.c
@@ -87,6 +87,7 @@ _PyAtExit_Init(PyInterpreterState *interp)
 
     state->callback_len = 32;
     state->ncallbacks = 0;
+    _Py_atomic_store_int(&state->handler_calling_started, 0);
     state->callbacks = PyMem_New(atexit_py_callback*, state->callback_len);
     if (state->callbacks == NULL) {
         return _PyStatus_NO_MEMORY();
@@ -118,10 +119,14 @@ _PyAtExit_Fini(PyInterpreterState *interp)
 
 
 static void
-atexit_callfuncs(struct atexit_state *state)
+atexit_callfuncs(struct atexit_state *state, int from_python_module)
 {
     assert(!PyErr_Occurred());
 
+    if (!from_python_module) {
+        _Py_atomic_store_int(&state->handler_calling_started, 1);
+    }
+
     if (state->ncallbacks == 0) {
         return;
     }
@@ -155,7 +160,7 @@ void
 _PyAtExit_Call(PyInterpreterState *interp)
 {
     struct atexit_state *state = &interp->atexit;
-    atexit_callfuncs(state);
+    atexit_callfuncs(state, 0);
 }
 
 
@@ -191,6 +196,29 @@ atexit_register(PyObject *module, PyObject *args, PyObject *kwargs)
     }
 
     struct atexit_state *state = get_atexit_state();
+    if (_Py_atomic_load_int(&state->handler_calling_started)) {
+        /* We could instead raise an error in this situation, but it is action
+         * at a distance. Not the callers fault. The culprit may be via a call
+         * chain invoking code transitively via an atexit handler or from
+         * within a thread spawned by an atexit handler.  See GH-113964.
+         */
+        if (PyErr_WarnEx(
+                PyExc_RuntimeWarning,
+                "atexit.register() ignored; atexit handler calls"
+                " have already started.",
+                1)) {
+            return NULL;
+        }
+        return Py_NewRef(func);
+    }
+
+    // Extremely unlikely, just to avoid overflow.
+    if (state->ncallbacks > INT_MAX/2048) {
+        PyErr_SetString(PyExc_RuntimeError,
+                        "Too many atexit handlers registered.");
+        return NULL;
+    }
+
     if (state->ncallbacks >= state->callback_len) {
         atexit_py_callback **r;
         state->callback_len += 16;
@@ -231,7 +259,7 @@ static PyObject *
 atexit_run_exitfuncs(PyObject *module, PyObject *unused)
 {
     struct atexit_state *state = get_atexit_state();
-    atexit_callfuncs(state);
+    atexit_callfuncs(state, 1);
     Py_RETURN_NONE;
 }
 

diff --git a/Modules/posixmodule.c b/Modules/posixmodule.c
@@ -7842,7 +7842,7 @@ os_fork1_impl(PyObject *module)
     pid_t pid;
 
     PyInterpreterState *interp = _PyInterpreterState_GET();
-    if (interp->finalizing) {
+    if (interp->started_finalization) {
         PyErr_SetString(PyExc_PythonFinalizationError,
                         "can't fork at interpreter shutdown");
         return NULL;
@@ -7886,7 +7886,7 @@ os_fork_impl(PyObject *module)
 {
     pid_t pid;
     PyInterpreterState *interp = _PyInterpreterState_GET();
-    if (interp->finalizing) {
+    if (interp->started_finalization) {
         PyErr_SetString(PyExc_PythonFinalizationError,
                         "can't fork at interpreter shutdown");
         return NULL;
@@ -8719,7 +8719,7 @@ os_forkpty_impl(PyObject *module)
     pid_t pid;
 
     PyInterpreterState *interp = _PyInterpreterState_GET();
-    if (interp->finalizing) {
+    if (interp->started_finalization) {
         PyErr_SetString(PyExc_PythonFinalizationError,
                         "can't fork at interpreter shutdown");
         return NULL;

diff --git a/Objects/unicodeobject.c b/Objects/unicodeobject.c
@@ -505,7 +505,7 @@ unicode_check_encoding_errors(const char *encoding, const char *errors)
 
     /* Disable checks during Python finalization. For example, it allows to
        call _PyObject_Dump() during finalization for debugging purpose. */
-    if (interp->finalizing) {
+    if (interp->started_finalization) {
         return 0;
     }
 

@@ -1877,27 +1877,27 @@ Py_FinalizeEx(void)
     // XXX assert(_Py_IsMainInterpreter(tstate->interp));
     // XXX assert(_Py_IsMainThread());
 
-    // Block some operations.
-    tstate->interp->finalizing = 1;
+    // Block some operations including fork().
+    tstate->interp->started_finalization = 1;
 
     // Wrap up existing "threading"-module-created, non-daemon threads.
     wait_for_thread_shutdown(tstate);
 
-    // Make any remaining pending calls.
     _Py_FinishPendingCalls(tstate);
 
     /* The interpreter is still entirely intact at this point, and the
-     * exit funcs may be relying on that.  In particular, if some thread
-     * or exit func is still waiting to do an import, the import machinery
-     * expects Py_IsInitialized() to return true.  So don't say the
-     * runtime is uninitialized until after the exit funcs have run.
-     * Note that Threading.py uses an exit func to do a join on all the
-     * threads created thru it, so this also protects pending imports in
-     * the threads created via Threading.
+     * atexit handlers are relying on that.  We cannot say the runtime is
+     * uninitialized until after the atexit handlers have run and and any
+     * non-daemon threads they spawned have been joined.
      */
-
     _PyAtExit_Call(tstate->interp);
 
+    if (tstate != tstate->interp->threads.head || tstate->next != NULL) {
+        // An atexit or signal handler must've spawned a thread; one last reap.
+        wait_for_thread_shutdown(tstate);
+        _Py_FinishPendingCalls(tstate);
+    }
+
     /* Copy the core config, PyInterpreterState_Delete() free
        the core config memory */
 #ifdef Py_REF_DEBUG
@@ -2259,18 +2259,23 @@ Py_EndInterpreter(PyThreadState *tstate)
     if (tstate->current_frame != NULL) {
         Py_FatalError("thread still has a frame");
     }
-    interp->finalizing = 1;
+    // Block some operations including fork().
+    interp->started_finalization = 1;
 
     // Wrap up existing "threading"-module-created, non-daemon threads.
     wait_for_thread_shutdown(tstate);
 
-    // Make any remaining pending calls.
     _Py_FinishPendingCalls(tstate);
 
     _PyAtExit_Call(tstate->interp);
 
     if (tstate != interp->threads.head || tstate->next != NULL) {
-        Py_FatalError("not the last thread");
+        // An atexit or signal handler must've spawned a thread; one last reap.
+        wait_for_thread_shutdown(tstate);
+        _Py_FinishPendingCalls(tstate);
+        if (tstate != interp->threads.head || tstate->next != NULL) {
+            Py_FatalError("not the last thread");
+        }
     }
 
     /* Remaining daemon threads will automatically exit