gh-123940: Ensure force-terminated daemon threads can be joined

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)

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):

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

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,37 @@ 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.
+        _PyEvent_Notify(&handle->thread_is_exiting);
+        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 +2527,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 +2622,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;
 }

Python/pylifecycle.c

@@ -75,6 +75,7 @@ static PyStatus init_sys_streams(PyThreadState *tstate);
 static PyStatus init_android_streams(PyThreadState *tstate);
 #endif
 static void wait_for_thread_shutdown(PyThreadState *tstate);
+static void daemon_threads_exited(PyThreadState *tstate);
 static void finalize_subinterpreters(void);
 static void call_ll_exitfuncs(_PyRuntimeState *runtime);
 
@@ -2040,6 +2041,7 @@ _Py_Finalize(_PyRuntimeState *runtime)
        before we call destructors. */
     PyThreadState *list = _PyThreadState_RemoveExcept(tstate);
     _PyEval_StartTheWorldAll(runtime);
+    daemon_threads_exited(tstate);
     _PyThreadState_DeleteList(list);
 
     /* At this point no Python code should be running at all.
@@ -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(tstate);
     // XXX Call something like _PyImport_Disable() here?
 
     _PyImport_FiniExternal(tstate->interp);
@@ -3350,6 +3353,31 @@ wait_for_thread_shutdown(PyThreadState *tstate)
     Py_DECREF(threading);
 }
 
+/* gh-123940: Mark remaining daemon threads as exited so that they may
+ * be joined from finalizers.
+ */
+static void
+daemon_threads_exited(PyThreadState *tstate)
+{
+    PyObject *threading = PyImport_GetModule(&_Py_ID(threading));
+    if (threading == NULL) {
+        if (_PyErr_Occurred(tstate)) {
+            PyErr_FormatUnraisable(
+                "Exception ignored while marking daemon threads exited");
+        }
+        /* else: threading not imported */
+        return;
+    }
+    PyObject *result =
+        PyObject_CallMethodNoArgs(threading, &_Py_ID(_daemon_threads_exited));
+    if (result == NULL) {
+        PyErr_FormatUnraisable(
+            "Exception ignored while marking daemon threads exited");
+    }
+    Py_XDECREF(result);
+    Py_DECREF(threading);
+}
+
 int Py_AtExit(void (*func)(void))
 {
     struct _atexit_runtime_state *state = &_PyRuntime.atexit;

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
 
 

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