gh-123940: Ensure force-terminated daemon threads can be joined by mpage · Pull Request #124150 · python/cpython

mpage · 2024-09-17T00:57:57Z

During finalization, daemon threads are forced to exit immediately:

Lines 2023 to 2026 in 10de360

    
               /* Remaining daemon threads will automatically exit 
        
                  when they attempt to take the GIL (ex: PyEval_RestoreThread()). */ 
        
               _PyInterpreterState_SetFinalizing(tstate->interp, tstate); 
        
               _PyRuntimeState_SetFinalizing(runtime, tstate);

without returning through the call-stack normally upon acquiring the GIL:

cpython/Python/ceval_gil.c

Lines 294 to 302 in 10de360

    
               if (_PyThreadState_MustExit(tstate)) { 
        
                   /* bpo-39877: If Py_Finalize() has been called and tstate is not the 
        
                      thread which called Py_Finalize(), exit immediately the thread. 
        
                      This code path can be reached by a daemon thread after Py_Finalize() 
        
                      completes. In this case, tstate is a dangling pointer: points to 
        
                      PyThreadState freed memory. */ 
        
                   PyThread_exit_thread(); 
        
               }

Finalizers that run after this must be able to join the forcefully terminated threads.

The current implementation notified of thread completion before returning from thread_run:

cpython/Modules/_threadmodule.c

Line 361 in 10de360

_PyEvent_Notify(&handle->thread_is_exiting);

Unfortunately, this code will never execute if the thread is forced to exit during finalization. Any code that attempts to join such a thread will block indefinitely.

To fix this, keep track of daemon threads and have the runtime notify their events after they are forced to exit.

Issue: Daemonic threads not killed in some circumstances in python 3.13 #123940

During finalization, daemon threads are force to exit immediately (without returning through the call-stack normally) upon acquiring the GIL. Finalizers that run after this must be able to join the forcefully terminated threads. The current implementation notified of thread completion before returning from `thread_run`. This code will never execute if the thread is forced to exit during finalization. Any code that attempts to join such a thread will block indefinitely. To fix this, use the old approach of notifying of thread completion when the thread state is cleared. This happens both when `thread_run` exits normally and when thread states are destroyed as part of finalization (which happens immediately after forcing daemon threads to exit, before any python code can run).

Python/pystate.c

Include/cpython/pystate.h

Include/internal/pycore_lock.h

Tools/tsan/suppressions_free_threading.txt

Lib/test/test_threading.py

colesbury · 2024-09-17T21:35:23Z

Lib/test/test_threading.py

+            def loop():
+                while True:
+                    pass


A slight variant where loop() calls time.sleep(1) hard crashes in 3.11 and 3.12. I'm not sure what to make of that, other than this sort of behavior wasn't robust previously either. I think it's noteworthy that the change that led to the issue was from just a few days ago -- it doesn't look like it was some longstanding code that just broke now.

possibly #87135 related.

This is the issue #116514 colesbury linked when I asked (I wasn't able to repro just the time.sleep variant mentioned here)

…_exited

…on_threads_exited

Python/pylifecycle.c

mpage · 2024-09-18T20:44:52Z

Marking this as a draft while I figure out the linker errors on windows.

Include threadmodule when building _freeze_module

mpage · 2024-09-18T22:22:34Z

@colesbury @ericsnowcurrently - Would you please have another look at this? I think it makes sense to move ThreadHandle from _threadmodule.c into pycore_pythread.h/Python/thread.c but that's a bigger change, so I went with a smaller fix here. I can do the refactor in a separate PR.

ericsnowcurrently

mostly LGTM

There's just the matter of where the code lives. It should mostly go in Python/thread.c. However, we have a bit of a mess in threadmodule.c already, which we needn't try to fix all in this PR. I might be okay with punting on that in favor of a follow-up PR, but I'd like the opinion of others on that first.

Modules/_threadmodule.c

ericsnowcurrently · 2024-09-26T21:24:32Z

Python/pystate.c

+    llist_init(&interp->threads.daemon_handles);
+    llist_init(&interp->threads.non_daemon_handles);


It would make sense to have a _PyThread_InitThreadHandles() for this, to match _PyThread_ClearThreadHandles().

Lib/test/test_threading.py

bedevere-app · 2024-09-26T21:44:25Z

A Python core developer has requested some changes be made to your pull request before we can consider merging it. If you could please address their requests along with any other requests in other reviews from core developers that would be appreciated.

Once you have made the requested changes, please leave a comment on this pull request containing the phrase I have made the requested changes; please review again. I will then notify any core developers who have left a review that you're ready for them to take another look at this pull request.

mpage · 2024-09-26T21:59:34Z

@ericsnowcurrently and I chatted in person and the consensus is that this PR is ok as-is, but we should follow it up with a refactor (only on main) to move functionality out of Modules/_threadmodule.c and into Python/thread.c.

gpshead · 2024-09-27T00:02:44Z

Unfortunately, this code will never execute if the thread is forced to exit during finalization. Any code that attempts to join such a thread will block indefinitely.

To fix this, keep track of daemon threads and have the runtime notify their events after they are forced to exit.

FYI - #105805, if merged, is set to change this so that these threads do not exit because our C API has no way for them to do so cleanly to cause an unwind of the call stack. They'll be made to hang instead.

encukou · 2025-06-09T11:37:33Z

And now with #130402, you get PythonFinalizationError when joining such a daemon thread (which is either sleeping in PyThread_hang_thread, or will be made to hang when it next tries to ~~acquire the GIL~~attach thread state).

It looks like there's nothing to do here, but, I'm not sure if someone still wants to go back to (and improve) force-killing rather than hanging.

@mpage, do you want to close the PR?

bedevere-app bot mentioned this pull request Sep 17, 2024

Daemonic threads not killed in some circumstances in python 3.13 #123940

Closed

mpage added the skip news label Sep 17, 2024

mpage added 3 commits September 16, 2024 18:42

Add suppression for _PyThreadState_MustExit

679ce7d

Merge branch 'main' into pythongh-123940

4d02a0b

Remove incorrect warnignore line

cd7b7d1

mpage requested review from colesbury and pitrou September 17, 2024 03:11

mpage marked this pull request as ready for review September 17, 2024 03:12

mpage requested a review from ericsnowcurrently as a code owner September 17, 2024 03:12

bedevere-app bot added the awaiting review label Sep 17, 2024

ericsnowcurrently reviewed Sep 17, 2024

View reviewed changes

Python/pystate.c Outdated Show resolved Hide resolved

Python/pystate.c Outdated Show resolved Hide resolved

Include/cpython/pystate.h Outdated Show resolved Hide resolved

Include/internal/pycore_lock.h Outdated Show resolved Hide resolved

colesbury reviewed Sep 17, 2024

View reviewed changes

mpage added 9 commits September 17, 2024 15:42

Assert that finalizer is called

bf186e2

Note the test that triggers the race in _PyThreadState_MustExit

96613ed

Merge branch 'main' into pythongh-123940

d18c872

Revert changes

33ab654

Mark all daemon threads as done during finalization

192b260

Merge branch 'main' into pythongh-123940

57a1c65

Only notify, don't mark handles as done

aa1039a

Document contract between wait_for_thread_shutdown and daemon_threads…

3978cc0

…_exited

Don't pass threading module between wait_for_thread_shutdown and daem…

5991fa4

…on_threads_exited

colesbury reviewed Sep 18, 2024

View reviewed changes

Python/pylifecycle.c Outdated Show resolved Hide resolved

Python/pylifecycle.c Outdated Show resolved Hide resolved

Don't call into Python to mark daemon threads completed

9a94905

mpage marked this pull request as draft September 18, 2024 20:44

bedevere-app bot removed the awaiting review label Sep 18, 2024

mpage added 2 commits September 18, 2024 13:57

Regen global objects

512b5fc

Maybe fix windows build

70f28c8

Include threadmodule when building _freeze_module

mpage marked this pull request as ready for review September 18, 2024 21:29

mpage requested a review from a team as a code owner September 18, 2024 21:29

bedevere-app bot added the awaiting review label Sep 18, 2024

gpshead mentioned this pull request Sep 26, 2024

pthread_exit & PyThread_exit_thread from PyEval_RestoreThread etc. are harmful #87135

Closed

ericsnowcurrently requested changes Sep 26, 2024

View reviewed changes

bedevere-app bot removed the awaiting review label Sep 26, 2024

bedevere-app bot added the awaiting changes label Sep 26, 2024

mpage closed this Jun 9, 2025

	/* Remaining daemon threads will automatically exit
	when they attempt to take the GIL (ex: PyEval_RestoreThread()). */
	_PyInterpreterState_SetFinalizing(tstate->interp, tstate);
	_PyRuntimeState_SetFinalizing(runtime, tstate);

	if (_PyThreadState_MustExit(tstate)) {
	/* bpo-39877: If Py_Finalize() has been called and tstate is not the
	thread which called Py_Finalize(), exit immediately the thread.

	This code path can be reached by a daemon thread after Py_Finalize()
	completes. In this case, tstate is a dangling pointer: points to
	PyThreadState freed memory. */
	PyThread_exit_thread();
	}

		llist_init(&interp->threads.daemon_handles);
		llist_init(&interp->threads.non_daemon_handles);

Uh oh!

Conversation

mpage commented Sep 17, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

colesbury Sep 17, 2024

Choose a reason for hiding this comment

Uh oh!

gpshead Sep 26, 2024

Choose a reason for hiding this comment

Uh oh!

hauntsaninja Sep 26, 2024

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

mpage commented Sep 18, 2024

Uh oh!

mpage commented Sep 18, 2024

Uh oh!

ericsnowcurrently left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

ericsnowcurrently Sep 26, 2024

Choose a reason for hiding this comment

Uh oh!

Uh oh!

bedevere-app bot commented Sep 26, 2024

Uh oh!

mpage commented Sep 26, 2024

Uh oh!

gpshead commented Sep 27, 2024

Uh oh!

encukou commented Jun 9, 2025

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

6 participants

mpage commented Sep 17, 2024 •

edited

Loading