From 192b260c7368ed9f2aa5fc1b9032e0f6690dc598 Mon Sep 17 00:00:00 2001 From: Matt Page Date: Tue, 17 Sep 2024 18:46:49 -0700 Subject: [PATCH] Mark all daemon threads as done during finalization --- .../pycore_global_objects_fini_generated.h | 1 + Include/internal/pycore_global_strings.h | 1 + .../internal/pycore_runtime_init_generated.h | 1 + .../internal/pycore_unicodeobject_generated.h | 4 + Lib/test/test_threading.py | 1 - Lib/threading.py | 1 + Modules/_threadmodule.c | 80 +++++++++++++++---- Python/pylifecycle.c | 31 +++++-- 8 files changed, 96 insertions(+), 24 deletions(-) diff --git a/Include/internal/pycore_global_objects_fini_generated.h b/Include/internal/pycore_global_objects_fini_generated.h index 6e948e16b7dbe8..0c8319520c5d21 100644 --- a/Include/internal/pycore_global_objects_fini_generated.h +++ b/Include/internal/pycore_global_objects_fini_generated.h @@ -745,6 +745,7 @@ _PyStaticObjects_CheckRefcnt(PyInterpreterState *interp) { _PyStaticObject_CheckRefcnt((PyObject *)&_Py_ID(_blksize)); _PyStaticObject_CheckRefcnt((PyObject *)&_Py_ID(_bootstrap)); _PyStaticObject_CheckRefcnt((PyObject *)&_Py_ID(_check_retval_)); + _PyStaticObject_CheckRefcnt((PyObject *)&_Py_ID(_daemon_threads_exited)); _PyStaticObject_CheckRefcnt((PyObject *)&_Py_ID(_dealloc_warn)); _PyStaticObject_CheckRefcnt((PyObject *)&_Py_ID(_feature_version)); _PyStaticObject_CheckRefcnt((PyObject *)&_Py_ID(_field_types)); diff --git a/Include/internal/pycore_global_strings.h b/Include/internal/pycore_global_strings.h index 5c63a6e519b93d..acd3a9b7043427 100644 --- a/Include/internal/pycore_global_strings.h +++ b/Include/internal/pycore_global_strings.h @@ -234,6 +234,7 @@ struct _Py_global_strings { STRUCT_FOR_ID(_blksize) STRUCT_FOR_ID(_bootstrap) STRUCT_FOR_ID(_check_retval_) + STRUCT_FOR_ID(_daemon_threads_exited) STRUCT_FOR_ID(_dealloc_warn) STRUCT_FOR_ID(_feature_version) STRUCT_FOR_ID(_field_types) diff --git a/Include/internal/pycore_runtime_init_generated.h b/Include/internal/pycore_runtime_init_generated.h index bac6b5b8fcfd9d..57d6eef73dbcf5 100644 --- a/Include/internal/pycore_runtime_init_generated.h +++ b/Include/internal/pycore_runtime_init_generated.h @@ -743,6 +743,7 @@ extern "C" { INIT_ID(_blksize), \ INIT_ID(_bootstrap), \ INIT_ID(_check_retval_), \ + INIT_ID(_daemon_threads_exited), \ INIT_ID(_dealloc_warn), \ INIT_ID(_feature_version), \ INIT_ID(_field_types), \ diff --git a/Include/internal/pycore_unicodeobject_generated.h b/Include/internal/pycore_unicodeobject_generated.h index efdbde4c8ea3c6..c4ef3bbbbee3cf 100644 --- a/Include/internal/pycore_unicodeobject_generated.h +++ b/Include/internal/pycore_unicodeobject_generated.h @@ -736,6 +736,10 @@ _PyUnicode_InitStaticStrings(PyInterpreterState *interp) { _PyUnicode_InternStatic(interp, &string); assert(_PyUnicode_CheckConsistency(string, 1)); assert(PyUnicode_GET_LENGTH(string) != 1); + string = &_Py_ID(_daemon_threads_exited); + _PyUnicode_InternStatic(interp, &string); + assert(_PyUnicode_CheckConsistency(string, 1)); + assert(PyUnicode_GET_LENGTH(string) != 1); string = &_Py_ID(_dealloc_warn); _PyUnicode_InternStatic(interp, &string); assert(_PyUnicode_CheckConsistency(string, 1)); diff --git a/Lib/test/test_threading.py b/Lib/test/test_threading.py index d32951a9ae55c0..a2db6cb5829942 100644 --- a/Lib/test/test_threading.py +++ b/Lib/test/test_threading.py @@ -1171,7 +1171,6 @@ 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 diff --git a/Lib/threading.py b/Lib/threading.py index 94ea2f08178369..14ea29c0cfab1c 100644 --- a/Lib/threading.py +++ b/Lib/threading.py @@ -42,6 +42,7 @@ get_ident = _thread.get_ident _get_main_thread_ident = _thread._get_main_thread_ident _is_main_interpreter = _thread._is_main_interpreter +_daemon_threads_exited = _thread._daemon_threads_exited try: get_native_id = _thread.get_native_id _HAVE_THREAD_NATIVE_ID = True diff --git a/Modules/_threadmodule.c b/Modules/_threadmodule.c index b3ed8e7bc56b9e..54d030adc4cda4 100644 --- a/Modules/_threadmodule.c +++ b/Modules/_threadmodule.c @@ -33,7 +33,11 @@ typedef struct { // 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; + struct llist_node non_daemon_handles; + + // Linked list of handles to daemon threads created by the threading + // module. We set the events in these handles as done at shutdown. + struct llist_node daemon_handles; } thread_module_state; static inline thread_module_state* @@ -78,7 +82,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` (see + // thread_module_state) struct llist_node shutdown_node; // The `ident`, `os_handle`, `has_os_handle`, and `state` fields are @@ -143,10 +148,11 @@ 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); } @@ -155,7 +161,10 @@ clear_shutdown_handles(thread_module_state *state) { HEAD_LOCK(&_PyRuntime); struct llist_node *node; - llist_for_each_safe(node, &state->shutdown_handles) { + llist_for_each_safe(node, &state->daemon_handles) { + llist_remove(node); + } + llist_for_each_safe(node, &state->non_daemon_handles) { llist_remove(node); } HEAD_UNLOCK(&_PyRuntime); @@ -378,7 +387,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 +399,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 +455,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; } @@ -1872,17 +1887,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 = &state->non_daemon_handles; + if (daemon) { + shutdown_handles = &state->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; } @@ -2372,7 +2382,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, &state->non_daemon_handles) { ThreadHandle *cur = llist_data(node, ThreadHandle, shutdown_node); if (cur->ident != ident) { ThreadHandle_incref(cur); @@ -2407,6 +2417,39 @@ PyDoc_STRVAR(shutdown_doc, \n\ Wait for all non-daemon threads (other than the calling thread) to stop."); +static PyObject * +thread__daemon_threads_exited(PyObject *self, PyObject *args) +{ + thread_module_state *state = get_thread_state(self); + + for (;;) { + HEAD_LOCK(&_PyRuntime); + if (llist_empty(&state->daemon_handles)) { + HEAD_UNLOCK(&_PyRuntime); + break; + } + struct llist_node *node = state->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. + if (ThreadHandle_set_done(handle) < 0) { + PyErr_WriteUnraisable(NULL); + } + ThreadHandle_decref(handle); + } + + Py_RETURN_NONE; +} + +PyDoc_STRVAR(_daemon_threads_exited_doc, +"_daemon_threads_exited($module, /)\n\ +--\n\ +\n\ +Mark that daemon threads have exited."); + static PyObject * thread__make_thread_handle(PyObject *module, PyObject *identobj) { @@ -2486,6 +2529,8 @@ static PyMethodDef thread_methods[] = { METH_NOARGS, thread__is_main_interpreter_doc}, {"_shutdown", thread_shutdown, METH_NOARGS, shutdown_doc}, + {"_daemon_threads_exited", thread__daemon_threads_exited, + METH_NOARGS, _daemon_threads_exited_doc}, {"_make_thread_handle", thread__make_thread_handle, METH_O, thread__make_thread_handle_doc}, {"_get_main_thread_ident", thread__get_main_thread_ident, @@ -2579,7 +2624,8 @@ thread_module_exec(PyObject *module) return -1; } - llist_init(&state->shutdown_handles); + llist_init(&state->daemon_handles); + llist_init(&state->non_daemon_handles); return 0; } diff --git a/Python/pylifecycle.c b/Python/pylifecycle.c index 27faf723745c21..5737f7c8b76528 100644 --- a/Python/pylifecycle.c +++ b/Python/pylifecycle.c @@ -74,7 +74,8 @@ static PyStatus init_sys_streams(PyThreadState *tstate); #ifdef __ANDROID__ static PyStatus init_android_streams(PyThreadState *tstate); #endif -static void wait_for_thread_shutdown(PyThreadState *tstate); +static PyObject *wait_for_thread_shutdown(PyThreadState *tstate); +static void daemon_threads_exited(PyObject *threading_module); static void finalize_subinterpreters(void); static void call_ll_exitfuncs(_PyRuntimeState *runtime); @@ -1985,7 +1986,7 @@ _Py_Finalize(_PyRuntimeState *runtime) tstate->interp->finalizing = 1; // Wrap up existing "threading"-module-created, non-daemon threads. - wait_for_thread_shutdown(tstate); + PyObject *threading_module = wait_for_thread_shutdown(tstate); // Make any remaining pending calls. _Py_FinishPendingCalls(tstate); @@ -2040,6 +2041,7 @@ _Py_Finalize(_PyRuntimeState *runtime) before we call destructors. */ PyThreadState *list = _PyThreadState_RemoveExcept(tstate); _PyEval_StartTheWorldAll(runtime); + daemon_threads_exited(threading_module); _PyThreadState_DeleteList(list); /* At this point no Python code should be running at all. @@ -2388,7 +2390,7 @@ Py_EndInterpreter(PyThreadState *tstate) interp->finalizing = 1; // Wrap up existing "threading"-module-created, non-daemon threads. - wait_for_thread_shutdown(tstate); + PyObject *threading_module = wait_for_thread_shutdown(tstate); // Make any remaining pending calls. _Py_FinishPendingCalls(tstate); @@ -2403,6 +2405,7 @@ Py_EndInterpreter(PyThreadState *tstate) when they attempt to take the GIL (ex: PyEval_RestoreThread()). */ _PyInterpreterState_SetFinalizing(interp, tstate); + daemon_threads_exited(threading_module); // XXX Call something like _PyImport_Disable() here? _PyImport_FiniExternal(tstate->interp); @@ -3328,7 +3331,7 @@ Py_ExitStatusException(PyStatus status) the threading module was imported in the first place. The shutdown routine will wait until all non-daemon "threading" threads have completed. */ -static void +static PyObject * wait_for_thread_shutdown(PyThreadState *tstate) { PyObject *result; @@ -3338,7 +3341,7 @@ wait_for_thread_shutdown(PyThreadState *tstate) PyErr_FormatUnraisable("Exception ignored on threading shutdown"); } /* else: threading not imported */ - return; + return NULL; } result = PyObject_CallMethodNoArgs(threading, &_Py_ID(_shutdown)); if (result == NULL) { @@ -3347,7 +3350,23 @@ wait_for_thread_shutdown(PyThreadState *tstate) else { Py_DECREF(result); } - Py_DECREF(threading); + return threading; +} + +static void +daemon_threads_exited(PyObject *threading_module) +{ + if (threading_module == NULL) { + return; + } + PyObject *result = PyObject_CallMethodNoArgs( + threading_module, &_Py_ID(_daemon_threads_exited)); + if (result == NULL) { + PyErr_FormatUnraisable( + "Exception ignored while marking daemon threads exited"); + } + Py_XDECREF(result); + Py_DECREF(threading_module); } int Py_AtExit(void (*func)(void))