diff --git a/Doc/library/atexit.rst b/Doc/library/atexit.rst index 43a8bd2d7cd133..c8396f02c6219a 100644 --- 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. diff --git a/Include/internal/pycore_atexit.h b/Include/internal/pycore_atexit.h index 4dcda8f517c787..a0b968f8fd4d4b 100644 --- a/Include/internal/pycore_atexit.h +++ b/Include/internal/pycore_atexit.h @@ -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 diff --git a/Include/internal/pycore_interp.h b/Include/internal/pycore_interp.h index 942f47340b3966..9710725bf5759d 100644 --- a/Include/internal/pycore_interp.h +++ b/Include/internal/pycore_interp.h @@ -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() diff --git a/Lib/multiprocessing/process.py b/Lib/multiprocessing/process.py index 271ba3fd325138..9cadd79a2be237 100644 --- a/Lib/multiprocessing/process.py +++ b/Lib/multiprocessing/process.py @@ -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 index f1dc12944cb6cc..b178a3c8a3a9f7 100644 --- a/Lib/test/test_threading.py +++ b/Lib/test/test_threading.py @@ -1155,12 +1155,13 @@ 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, ()) @@ -1168,8 +1169,26 @@ def exit_handler(): 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 index 31ab77c92b1c20..cad8c3c79290ac 100644 --- 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 new file mode 100644 index 00000000000000..c22093fc5b3879 --- /dev/null +++ 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. diff --git a/Modules/_posixsubprocess.c b/Modules/_posixsubprocess.c index bcbbe70680b8e7..e3a2eab2ccf8eb 100644 --- a/Modules/_posixsubprocess.c +++ b/Modules/_posixsubprocess.c @@ -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 index 6889e8f6e12126..ddbe0a026ed828 100644 --- 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 index b6f1bcbca67916..10b56a774e3fa8 100644 --- 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 index 7b2d3661ee5546..8874d49524dd6e 100644 --- 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 index c8f647a7a71135..99180baa642d23 100644 --- 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; } diff --git a/Python/pylifecycle.c b/Python/pylifecycle.c index 3a2c0a450ac9d9..fb0642ba6337f3 100644 --- a/Python/pylifecycle.c +++ b/Python/pylifecycle.c @@ -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