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

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

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

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

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,15 +2368,15 @@ 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;
 
  // 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;
 }
 

PCbuild/_freeze_module.vcxproj

@@ -111,6 +111,7 @@
  <ClCompile Include="..\Modules\getpath_noop.c" />
  <ClCompile Include="..\Modules\posixmodule.c" />
  <ClCompile Include="..\Modules\signalmodule.c" />
+ <ClCompile Include="..\Modules\_threadmodule.c" />
  <ClCompile Include="..\Modules\timemodule.c" />
  <ClCompile Include="..\Modules\_tracemalloc.c" />
  <ClCompile Include="..\Modules\_io\_iomodule.c" />

PCbuild/_freeze_module.vcxproj.filters

@@ -431,6 +431,9 @@
  <ClCompile Include="..\Python\thread.c">
  <Filter>Source Files</Filter>
  </ClCompile>
+ <ClCompile Include="..\Modules\_threadmodule.c">
+ <Filter>Source Files</Filter>
+ </ClCompile>
  <ClCompile Include="..\Modules\timemodule.c">
  <Filter>Source Files</Filter>
  </ClCompile>

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.

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

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