gh-113964: Restore the ability to start threads from atexit handler functions. #116982

gpshead · 2024-03-19T08:51:38Z

In 3.12 we removed the ability to start threads from within an atexit.register() handler function context as Python interpreter finalization was starting.

Users were relying on this, sometimes indirectly, for reasonable seeming reasons.

This restores that ability by doing one final thread joining cleanup pass after atexit handlers if any threads are found to remain.

The unittests altered in sibling CL #116677 that this PR modifies and builds upon should be retained.

Ideally we'd also backport this to 3.12 as a bugfix to restore that ability. But that backport may be challenging.

Issue: 3.12 regression: "can't create new thread at interpreter shutdown" from non-daemon threads or atexit handlers #113964

📚 Documentation preview 📚: https://cpython-previews--116982.org.readthedocs.build/

vstinner · 2024-03-19T09:10:13Z

Include/internal/pycore_interp.h

@@ -102,7 +102,7 @@ struct _is {
       In order to be effective, this must be set to 0 during or right
       after allocation. */
    int _initialized;
-    int finalizing;
+    int started_finalization;  /* set early, used to block some actions. */


Can you give some examples here of actions which are blocked?

Depending upon implementation i'm not sure we even want this flag anymore... If we allow threading and forking to happen up until the modern _finalizing API returns False there'd be no need for it.

Otherwise "some actions" needing describing is basically those two and an odd debug thing in unicodeobject.c as far the users of this go. Every place that touches it can be seen in this PR.

gpshead · 2024-03-19T08:58:19Z

Include/internal/pycore_interp.h

@@ -102,7 +102,7 @@ struct _is {
       In order to be effective, this must be set to 0 during or right
       after allocation. */
    int _initialized;
-    int finalizing;
+    int started_finalization;  /* set early, used to block some actions. */


This seemed to be a more accurate name given what it gets used for and the below more comprehensive _finalizing with an API around it.

gpshead · 2024-03-19T09:04:40Z

Lib/threading.py

-    # dubious, but some code does it. We can't wait for it to be marked as done
-    # normally - that won't happen until the interpreter is nearly dead. So
-    # mark it done here.
-    if _main_thread._handle.is_done() and _is_main_interpreter():


This early return logic started in ee84a60 but no longer seems relevant in 3.13 given the other refactoring in threading.py cleaning up the locking mess.

gpshead · 2024-03-19T09:05:55Z

Lib/threading.py

    global _SHUTTING_DOWN
    _SHUTTING_DOWN = True

    # Call registered threading atexit functions before threads are joined.
    # Order is reversed, similar to atexit.
-    for atexit_call in reversed(_threading_atexits):
+    while _threading_atexits and (atexit_call := _threading_atexits.pop()):


_shutdown can be called twice given the re-join after atexit handlers if other threads are found. removing them as they're called prevents future calls. I should turn this into a comment. test_atexit already has tests that cover this situation.

gpshead · 2024-03-19T09:08:55Z

Modules/atexitmodule.c

@@ -191,6 +196,29 @@ atexit_register(PyObject *module, PyObject *args, PyObject *kwargs)
    }

    struct atexit_state *state = get_atexit_state();
+    if (_Py_atomic_load_int(&state->handler_calling_started)) {


arguably all of the logic around blocking new atexit.registers from happening could be left out of this PR. it felt relevant though. we never really defined or documented what happened before (registrations made from a handler were never called). This makes it explicit and gives some indication as to when a registration is being ignored.

gpshead · 2024-03-19T09:09:42Z

Modules/atexitmodule.c

 {
    assert(!PyErr_Occurred());

+    if (!from_python_module) {


because Lib/_test_atexit.py calls atexit._run_callbacks() which shouldn't set the flag.

gpshead · 2024-03-19T09:10:37Z

Python/pylifecycle.c

There's a lot of duplication between Py_FinalizeEx and PyEnd_Interpreter but this PR is not the place to reconcile any of that.

colesbury

I think people who start threads from at-exit handlers are asking for trouble, but I would lean towards not waiting on any threads started by at-exit handlers. In Py_FinalizeEx we starting reaping thread states immediately after calling at exit handlers.

Terminating an application with threads now requires two ctrl-cs! (Try out https://gist.github.com/colesbury/4093e5675d988ceea10fb7eda4c8c864)
It's closer to the pre-3.12 behavior
It's also what Java does. While we in no way have to copy Java's behavior, I think it's worth considering how other VMs handle similar challenges. (Try out https://gist.github.com/colesbury/5a4ce123f1917c097bd5b45735e89d79)

I think Py_EndInterpreter needs a bit of work (but like you said, not in this PR). Pressing ctrl-c with something that calls Py_EndInterpreter can lead to Py_FatalError("not the last thread").

colesbury · 2024-03-19T19:21:47Z

Python/pylifecycle.c

    _PyAtExit_Call(tstate->interp);

+    if (tstate != tstate->interp->threads.head || tstate->next != NULL) {


I think if you are accessing tstate->interp->threads.head or tstate->next you need to be within a HEAD_LOCK(&_PyRuntime);.

gpshead · 2024-03-20T00:08:08Z

I would lean towards not waiting on any threads started by at-exit handlers.

If we don't wait on them, that's basically turning them into daemon threads which are running despite us tearing down the interpreter underneath them. That also feels wrong (the entire concept of daemon threads having been a mistake).

I'll go back to the user reported issues and try to pick out which actual use case feel supportable.

...

Sleeping on this change I think even waiting once isn't enough if we're going to wait again: So long as thread creation is allowed, threads could spawn more threads which might not be in the set threading._shutdown() joins. So some could be un-joined anyways. Multiple Ctrl-C's being needed in this situation is indeed a serious problem with re-waiting though. Unless I set a flag to disallow new thread creation before the final thread shutdown join. This really either needs to be a while loop waiting for there to be no other threads, or just throwing up our hands and pulling the runtime out from underneath them. (that whole "not the last thread" thing in Py_EndInterpreter also feeling suspicious)

There might be another compromise? Don't re-wait if an exception was set after the initial _Py_FinishPendingCalls (KeyboardInterrupt from Ctrl-C for example). atexit handlers still get run, but if we're on the fast path to exit we shouldn't wait for threads they spawn?

more to ponder.

Restore the ability to start threads from atexit handler functions.

150a2d7

gpshead requested a review from colesbury March 19, 2024 08:51

gpshead requested a review from ericsnowcurrently as a code owner March 19, 2024 08:51

bedevere-app bot added the awaiting core review label Mar 19, 2024

bedevere-app bot mentioned this pull request Mar 19, 2024

3.12 regression: "can't create new thread at interpreter shutdown" from non-daemon threads or atexit handlers #113964

Open

gpshead requested review from ericsnowcurrently and removed request for ericsnowcurrently March 19, 2024 08:51

gpshead mentioned this pull request Mar 19, 2024

gh-113964: Don't prevent new threads until all non-daemon threads exit #116677

Merged

gpshead requested a review from vstinner March 19, 2024 08:57

Fix duplicate test method name and _thread test.

1ba58cd

vstinner reviewed Mar 19, 2024

View reviewed changes

gpshead commented Mar 19, 2024

View reviewed changes

colesbury reviewed Mar 19, 2024

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

gh-113964: Restore the ability to start threads from atexit handler functions. #116982

gh-113964: Restore the ability to start threads from atexit handler functions. #116982

gpshead commented Mar 19, 2024 •

edited

Loading

vstinner Mar 19, 2024

gpshead Mar 19, 2024

gpshead Mar 19, 2024

gpshead Mar 19, 2024

gpshead Mar 19, 2024

gpshead Mar 19, 2024

gpshead Mar 19, 2024

gpshead Mar 19, 2024

colesbury left a comment

colesbury Mar 19, 2024

gpshead commented Mar 20, 2024 •

edited

Loading

		_PyAtExit_Call(tstate->interp);

		if (tstate != tstate->interp->threads.head \|\| tstate->next != NULL) {

gh-113964: Restore the ability to start threads from atexit handler functions. #116982

Are you sure you want to change the base?

gh-113964: Restore the ability to start threads from atexit handler functions. #116982

Conversation

gpshead commented Mar 19, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

colesbury left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

gpshead commented Mar 20, 2024 • edited Loading

gpshead commented Mar 19, 2024 •

edited

Loading

gpshead commented Mar 20, 2024 •

edited

Loading