GeezerVison

The World Wide Web, restored to 800 pixels of glory.

GeezerVison is showing github.com — gh-113964: Restore the ability to start threads from atexit handler functions. by gpshead · Pull Request #116982 · python/cpython

Conversation

gpshead commented Mar 19, 2024 •

edited

Loading

Uh oh!

There was an error while loading. Please reload this page.

Copy link

Copy Markdown

Member

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/

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

All reactions


          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

Comment thread

Include/internal/pycore_interp.h

		after allocation. */
		int _initialized;
		int finalizing;
		int started_finalization; /* set early, used to block some actions. */

vstinner Mar 19, 2024

Copy link

Copy Markdown

Member

There was a problem hiding this comment.

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

All reactions

gpshead Mar 19, 2024

Copy link

Copy Markdown

Member Author

There was a problem hiding this comment.

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

All reactions

gpshead commented

Mar 19, 2024

View reviewed changes

Comment thread

Include/internal/pycore_interp.h

		after allocation. */
		int _initialized;
		int finalizing;
		int started_finalization; /* set early, used to block some actions. */

gpshead Mar 19, 2024

Copy link

Copy Markdown

Member Author

There was a problem hiding this comment.

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

colesbury reacted with thumbs up emoji

All reactions

👍 1 reaction

Comment thread

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

gpshead Mar 19, 2024

Copy link

Copy Markdown

Member Author

There was a problem hiding this comment.

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

All reactions

Comment thread

Lib/threading.py

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

gpshead Mar 19, 2024

Copy link

Copy Markdown

Member Author

There was a problem hiding this comment.

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

_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.

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

All reactions

Comment thread

Modules/atexitmodule.c

		}

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

gpshead Mar 19, 2024

Copy link

Copy Markdown

Member Author

There was a problem hiding this comment.

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

All reactions

Comment thread

Modules/atexitmodule.c

		{
		assert(!PyErr_Occurred());

		if (!from_python_module) {

gpshead Mar 19, 2024

Copy link

Copy Markdown

Member Author

There was a problem hiding this comment.

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

All reactions

Comment thread

Python/pylifecycle.c

gpshead Mar 19, 2024

Copy link

Copy Markdown

Member Author

There was a problem hiding this comment.

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

All reactions

colesbury reviewed

Mar 19, 2024

View reviewed changes

colesbury left a comment

Copy link

Copy Markdown

Contributor

There was a problem hiding this comment.

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

gpshead reacted with thumbs up emoji

All reactions

👍 1 reaction

Comment thread

Python/pylifecycle.c


		_PyAtExit_Call(tstate->interp);

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

colesbury Mar 19, 2024

Copy link

Copy Markdown

Contributor

There was a problem hiding this comment.

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

gpshead reacted with thumbs up emoji

All reactions

👍 1 reaction

gpshead commented Mar 20, 2024 •

edited

Loading

Uh oh!

There was an error while loading. Please reload this page.

Copy link

Copy Markdown

Member Author

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.

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

github-actions Bot commented Apr 15, 2026

Copy link

Copy Markdown

This PR is stale because it has been open for 30 days with no activity.

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

github-actions Bot added the stale Stale PR or inactive for long period of time. label

Apr 15, 2026

Reviewers

vstinner

vstinner left review comments

colesbury

colesbury left review comments

ericsnowcurrently

Awaiting requested review from ericsnowcurrently

ericsnowcurrently is a code owner

Uh oh!

There was an error while loading. Please reload this page.

Assignees

No one assigned

Labels

awaiting core review stale

Stale PR or inactive for long period of time.

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

Uh oh!

There was an error while loading. Please reload this page.

3 participants

Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.

You are visitor 01402503 — GeezerVison does not store the pages you look up.
Best viewed with Netscape Navigator 4.7 or Internet Explorer 6 at 800×600. No cookies required.

Conversation

gpshead commented Mar 19, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

colesbury left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

gpshead commented Mar 20, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

github-actions Bot commented Apr 15, 2026

Uh oh!

Reviewers

Uh oh!

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

gpshead commented Mar 19, 2024 •

edited

Loading

Uh oh!

There was an error while loading. Please reload this page.

gpshead commented Mar 20, 2024 •

edited

Loading

Uh oh!

There was an error while loading. Please reload this page.