diff --git a/Include/internal/pycore_interp.h b/Include/internal/pycore_interp.h index a1c1dd0c957230..6ef7079ac6fc4a 100644 --- a/Include/internal/pycore_interp.h +++ b/Include/internal/pycore_interp.h @@ -139,6 +139,12 @@ struct _is { or the size specified by the THREAD_STACK_SIZE macro. */ /* Used in Python/thread.c. */ size_t stacksize; + + /* Linked lists of ThreadHandles for threads created by + the threading module. + */ + struct llist_node non_daemon_handles; + struct llist_node daemon_handles; } threads; /* Reference to the _PyRuntime global variable. This field exists diff --git a/Include/internal/pycore_pythread.h b/Include/internal/pycore_pythread.h index f3f5942444e851..16fa05da85b3cd 100644 --- a/Include/internal/pycore_pythread.h +++ b/Include/internal/pycore_pythread.h @@ -95,6 +95,8 @@ extern int _PyThread_at_fork_reinit(PyThread_type_lock *lock); extern void _PyThread_AfterFork(struct _pythread_runtime_state *state); #endif /* HAVE_FORK */ +extern void _PyThread_DaemonThreadsForceKilled(PyInterpreterState *interp); +extern void _PyThread_ClearThreadHandles(PyInterpreterState *interp); // unset: -1 seconds, in nanoseconds #define PyThread_UNSET_TIMEOUT ((PyTime_t)(-1 * 1000 * 1000 * 1000)) diff --git a/Lib/test/test_threading.py b/Lib/test/test_threading.py index 329767aa82e336..d32951a9ae55c0 100644 --- a/Lib/test/test_threading.py +++ b/Lib/test/test_threading.py @@ -1171,6 +1171,41 @@ def __del__(self): self.assertEqual(out.strip(), b"OK") self.assertIn(b"can't create new thread at interpreter shutdown", err) + @unittest.skipIf(support.Py_GIL_DISABLED, "gh-124149: daemon threads don't force exit") + def test_join_force_terminated_daemon_thread_in_finalization(self): + # gh-123940: Py_Finalize() forces all daemon threads to exit + # immediately (without unwinding the stack) upon acquiring the + # GIL. Finalizers that run after this must be able to join the daemon + # threads that were forced to exit. + code = textwrap.dedent(""" + import threading + + + def loop(): + while True: + pass + + + class Cycle: + def __init__(self): + self.self_ref = self + self.thr = threading.Thread(target=loop, daemon=True) + self.thr.start() + + def __del__(self): + self.thr.join() + print('__del__ called') + + # Cycle holds a reference to itself, which ensures it is cleaned + # up during the GC that runs after daemon threads have been + # forced to exit during finalization. + Cycle() + """) + rc, out, err = assert_python_ok("-c", code) + self.assertEqual(err, b"") + self.assertIn(b"__del__ called", out) + + class ThreadJoinOnShutdown(BaseTestCase): def _run_and_join(self, script): diff --git a/Modules/_threadmodule.c b/Modules/_threadmodule.c index b3ed8e7bc56b9e..25c59a00479df0 100644 --- a/Modules/_threadmodule.c +++ b/Modules/_threadmodule.c @@ -30,10 +30,6 @@ typedef struct { PyTypeObject *local_type; PyTypeObject *local_dummy_type; PyTypeObject *thread_handle_type; - - // Linked list of handles to all non-daemon threads created by the - // threading module. We wait for these to finish at shutdown. - struct llist_node shutdown_handles; } thread_module_state; static inline thread_module_state* @@ -78,7 +74,8 @@ typedef enum { typedef struct { struct llist_node node; // linked list node (see _pythread_runtime_state) - // linked list node (see thread_module_state) + // belongs to either `non_daemon_handles` or `daemon_handles` on + // PyInterpreterState struct llist_node shutdown_node; // The `ident`, `os_handle`, `has_os_handle`, and `state` fields are @@ -143,19 +140,26 @@ ThreadHandle_get_os_handle(ThreadHandle *handle, PyThread_handle_t *os_handle) } static void -add_to_shutdown_handles(thread_module_state *state, ThreadHandle *handle) +add_to_shutdown_handles(struct llist_node *shutdown_handles, + ThreadHandle *handle) { HEAD_LOCK(&_PyRuntime); - llist_insert_tail(&state->shutdown_handles, &handle->shutdown_node); + llist_insert_tail(shutdown_handles, &handle->shutdown_node); HEAD_UNLOCK(&_PyRuntime); } -static void -clear_shutdown_handles(thread_module_state *state) +// Remove any remaining handles (e.g. if shutdown exited early due to +// interrupt) so that attempts to unlink the handle after our module state +// is destroyed do not crash. +void +_PyThread_ClearThreadHandles(PyInterpreterState *interp) { HEAD_LOCK(&_PyRuntime); struct llist_node *node; - llist_for_each_safe(node, &state->shutdown_handles) { + llist_for_each_safe(node, &interp->threads.daemon_handles) { + llist_remove(node); + } + llist_for_each_safe(node, &interp->threads.non_daemon_handles) { llist_remove(node); } HEAD_UNLOCK(&_PyRuntime); @@ -378,7 +382,7 @@ force_done(ThreadHandle *handle) static int ThreadHandle_start(ThreadHandle *self, PyObject *func, PyObject *args, - PyObject *kwargs) + PyObject *kwargs, struct llist_node *shutdown_handles) { // Mark the handle as starting to prevent any other threads from doing so PyMutex_Lock(&self->mutex); @@ -390,6 +394,11 @@ ThreadHandle_start(ThreadHandle *self, PyObject *func, PyObject *args, self->state = THREAD_HANDLE_STARTING; PyMutex_Unlock(&self->mutex); + // Add the handle before starting the thread to avoid adding a handle + // to a thread that has already finished (i.e. if the thread finishes + // before the call to `ThreadHandle_start()` below returns). + add_to_shutdown_handles(shutdown_handles, self); + // Do all the heavy lifting outside of the mutex. All other operations on // the handle should fail since the handle is in the starting state. @@ -441,6 +450,7 @@ ThreadHandle_start(ThreadHandle *self, PyObject *func, PyObject *args, return 0; start_failed: + remove_from_shutdown_handles(self); _PyOnceFlag_CallOnce(&self->once, (_Py_once_fn_t *)force_done, self); return -1; } @@ -1857,8 +1867,8 @@ Return True if daemon threads are allowed in the current interpreter,\n\ and False otherwise.\n"); static int -do_start_new_thread(thread_module_state *state, PyObject *func, PyObject *args, - PyObject *kwargs, ThreadHandle *handle, int daemon) +do_start_new_thread(PyObject *func, PyObject *args, PyObject *kwargs, + ThreadHandle *handle, int daemon) { PyInterpreterState *interp = _PyInterpreterState_GET(); if (!_PyInterpreterState_HasFeature(interp, Py_RTFLAGS_THREADS)) { @@ -1872,17 +1882,12 @@ do_start_new_thread(thread_module_state *state, PyObject *func, PyObject *args, return -1; } - if (!daemon) { - // Add the handle before starting the thread to avoid adding a handle - // to a thread that has already finished (i.e. if the thread finishes - // before the call to `ThreadHandle_start()` below returns). - add_to_shutdown_handles(state, handle); + struct llist_node *shutdown_handles = &interp->threads.non_daemon_handles; + if (daemon) { + shutdown_handles = &interp->threads.daemon_handles; } - if (ThreadHandle_start(handle, func, args, kwargs) < 0) { - if (!daemon) { - remove_from_shutdown_handles(handle); - } + if (ThreadHandle_start(handle, func, args, kwargs, shutdown_handles) < 0) { return -1; } @@ -1893,7 +1898,6 @@ static PyObject * thread_PyThread_start_new_thread(PyObject *module, PyObject *fargs) { PyObject *func, *args, *kwargs = NULL; - thread_module_state *state = get_thread_state(module); if (!PyArg_UnpackTuple(fargs, "start_new_thread", 2, 3, &func, &args, &kwargs)) @@ -1924,8 +1928,7 @@ thread_PyThread_start_new_thread(PyObject *module, PyObject *fargs) return NULL; } - int st = - do_start_new_thread(state, func, args, kwargs, handle, /*daemon=*/1); + int st = do_start_new_thread(func, args, kwargs, handle, /*daemon=*/1); if (st < 0) { ThreadHandle_decref(handle); return NULL; @@ -2002,8 +2005,9 @@ thread_PyThread_start_joinable_thread(PyObject *module, PyObject *fargs, if (args == NULL) { return NULL; } - int st = do_start_new_thread(state, func, args, - /*kwargs=*/ NULL, ((PyThreadHandleObject*)hobj)->handle, daemon); + int st = do_start_new_thread( + func, args, + /*kwargs=*/NULL, ((PyThreadHandleObject *)hobj)->handle, daemon); Py_DECREF(args); if (st < 0) { Py_DECREF(hobj); @@ -2364,7 +2368,7 @@ static PyObject * thread_shutdown(PyObject *self, PyObject *args) { PyThread_ident_t ident = PyThread_get_thread_ident_ex(); - thread_module_state *state = get_thread_state(self); + PyInterpreterState *interp = _PyInterpreterState_GET(); for (;;) { ThreadHandle *handle = NULL; @@ -2372,7 +2376,7 @@ thread_shutdown(PyObject *self, PyObject *args) // Find a thread that's not yet finished. HEAD_LOCK(&_PyRuntime); struct llist_node *node; - llist_for_each_safe(node, &state->shutdown_handles) { + llist_for_each_safe(node, &interp->threads.non_daemon_handles) { ThreadHandle *cur = llist_data(node, ThreadHandle, shutdown_node); if (cur->ident != ident) { ThreadHandle_incref(cur); @@ -2407,6 +2411,30 @@ PyDoc_STRVAR(shutdown_doc, \n\ Wait for all non-daemon threads (other than the calling thread) to stop."); +/* gh-123940: Mark remaining daemon threads as exited so that they may + * be joined from finalizers. + */ +void +_PyThread_DaemonThreadsForceKilled(PyInterpreterState *interp) +{ + for (;;) { + HEAD_LOCK(&_PyRuntime); + if (llist_empty(&interp->threads.daemon_handles)) { + HEAD_UNLOCK(&_PyRuntime); + break; + } + struct llist_node *node = interp->threads.daemon_handles.next; + ThreadHandle *handle = llist_data(node, ThreadHandle, shutdown_node); + ThreadHandle_incref(handle); + llist_remove(node); + HEAD_UNLOCK(&_PyRuntime); + // gh-123940: Mark daemon threads as done so that they can be joined + // from finalizers. + _PyEvent_Notify(&handle->thread_is_exiting); + ThreadHandle_decref(handle); + } +} + static PyObject * thread__make_thread_handle(PyObject *module, PyObject *identobj) { @@ -2579,8 +2607,6 @@ thread_module_exec(PyObject *module) return -1; } - llist_init(&state->shutdown_handles); - return 0; } @@ -2606,10 +2632,6 @@ thread_module_clear(PyObject *module) Py_CLEAR(state->local_type); Py_CLEAR(state->local_dummy_type); Py_CLEAR(state->thread_handle_type); - // Remove any remaining handles (e.g. if shutdown exited early due to - // interrupt) so that attempts to unlink the handle after our module state - // is destroyed do not crash. - clear_shutdown_handles(state); return 0; } diff --git a/PCbuild/_freeze_module.vcxproj b/PCbuild/_freeze_module.vcxproj index 743e6e2a66a8f1..c2894c5eb63c3e 100644 --- a/PCbuild/_freeze_module.vcxproj +++ b/PCbuild/_freeze_module.vcxproj @@ -111,6 +111,7 @@ + diff --git a/PCbuild/_freeze_module.vcxproj.filters b/PCbuild/_freeze_module.vcxproj.filters index 0887a47917a931..26975767a339f7 100644 --- a/PCbuild/_freeze_module.vcxproj.filters +++ b/PCbuild/_freeze_module.vcxproj.filters @@ -431,6 +431,9 @@ Source Files + + Source Files + Source Files diff --git a/Python/pylifecycle.c b/Python/pylifecycle.c index 27faf723745c21..89bf1aa117ef81 100644 --- a/Python/pylifecycle.c +++ b/Python/pylifecycle.c @@ -2040,6 +2040,7 @@ _Py_Finalize(_PyRuntimeState *runtime) before we call destructors. */ PyThreadState *list = _PyThreadState_RemoveExcept(tstate); _PyEval_StartTheWorldAll(runtime); + _PyThread_DaemonThreadsForceKilled(tstate->interp); _PyThreadState_DeleteList(list); /* At this point no Python code should be running at all. diff --git a/Python/pystate.c b/Python/pystate.c index 54caf373e91d6c..24a0b2558c677e 100644 --- a/Python/pystate.c +++ b/Python/pystate.c @@ -665,6 +665,8 @@ init_interpreter(PyInterpreterState *interp, /* Fix the self-referential, statically initialized fields. */ interp->dtoa = (struct _dtoa_state)_dtoa_state_INIT(interp); } + llist_init(&interp->threads.daemon_handles); + llist_init(&interp->threads.non_daemon_handles); interp->_initialized = 1; return _PyStatus_OK(); @@ -811,7 +813,7 @@ interpreter_clear(PyInterpreterState *interp, PyThreadState *tstate) // XXX Eliminate the need to do this. tstate->_status.cleared = 0; } - + _PyThread_ClearThreadHandles(interp); #ifdef _Py_TIER2 _PyOptimizerObject *old = _Py_SetOptimizer(interp, NULL); assert(old != NULL); diff --git a/Tools/tsan/suppressions_free_threading.txt b/Tools/tsan/suppressions_free_threading.txt index e5eb665ae212de..b99597c0421651 100644 --- a/Tools/tsan/suppressions_free_threading.txt +++ b/Tools/tsan/suppressions_free_threading.txt @@ -9,6 +9,11 @@ race:get_allocator_unlocked race:set_allocator_unlocked +# Triggered by test_threading.ThreadTests.test_join_force_terminated_daemon_thread_in_finalization +# https://gist.github.com/mpage/ff1c7094bc8237d98387678d5f52058f +race_top:_PyThreadState_MustExit + + ## Free-threaded suppressions diff --git a/Tools/tsan/supressions.txt b/Tools/tsan/supressions.txt index 22ba9d6ba2ab4d..c20ad26e95077e 100644 --- a/Tools/tsan/supressions.txt +++ b/Tools/tsan/supressions.txt @@ -3,5 +3,9 @@ race:get_allocator_unlocked race:set_allocator_unlocked +# Triggered by test_threading.ThreadTests.test_join_force_terminated_daemon_thread_in_finalization +# https://gist.github.com/mpage/ff1c7094bc8237d98387678d5f52058f +race_top:_PyThreadState_MustExit + # https://gist.github.com/mpage/daaf32b39180c1989572957b943eb665 thread:pthread_create