GH-125603: Don't count executing generators and coroutines as referrers in gc.gc_referrers. #125640

markshannon · 2024-10-17T10:54:26Z

Skipping news as this is reverting behavior that never made it into a release.

Issue: [3.14] change in behaviour in gc.get_referrers(some_local) #125603

…t_referrers.

kumaraditya303

asyncio changes LGTM

colesbury · 2024-10-17T14:32:59Z

Objects/genobject.c

@@ -58,7 +58,7 @@ gen_traverse(PyObject *self, visitproc visit, void *arg)
    PyGenObject *gen = _PyGen_CAST(self);
    Py_VISIT(gen->gi_name);
    Py_VISIT(gen->gi_qualname);
-    if (gen->gi_frame_state != FRAME_CLEARED) {
+    if (gen->gi_frame_state < FRAME_EXECUTING) {


I think this is okay, but it also adds some additional constraints that will make the GC in the free threading build more fragile. I think we have to make sure that any gen/coro marked as FRAME_EXECUTING is on a PyThreadState's frame stack -- otherwise some deferred references may not be visible to the GC and collected while still in use.

There must not be any escaping calls between setting gi_frame_state = FRAME_EXECUTING and pushing the frame to the thread's stack.

There must not be any escaping calls between popping the frame from the stack and setting gi_frame_state to some other value. (i.e., exit_unwind -> clear_gen_frame)

All those things are already true, because we rely on gi_frame_state == FRAME_EXECUTING to guard against sending to an already executing generator.

At shutdown we delete all PyThreadStates except for the main thread. Those threads could be running generators or coroutines stuck in some long running call (like a time.sleep()). We run the GC after that. That seems like it would be unsafe (because we're now hiding deferred _PyStackRef from the GC.

This test program segfaults in the free threading build with this PR:

https://gist.github.com/colesbury/11d59b9987e881a3c016b086bb4ba1ff

If the generator is executing, it is part of the stack, not the heap.
So, I think tp_traverse should not be traversing executing generators.

@colesbury
How does the free-threading GC find references that are on the stack (in normal frames)?
We should do that for executing generators.

Yes, during shutdown, we delete all PyThreadStates except for the main thread.

Let me rephrase:
When deleting the thread state, how do we clean up the references?

Presumably, we should be changing the state of the generator when we clean up its frame but we aren't.

To answer my own question. We don't cleanup the references 🙁

We call PyThreadState_Clear() on the deleted thread states, but that doesn't clean up current_frame. We don't call PyThreadState_Delete() for reasons that are not clear to me. Even if PyThreadState_Clear() cleaned up current_frame that wouldn't be sufficient because we unlink all the thread states before calling PyThreadState_Clear() and (by your definition) that already puts them in an invalid state.

This is all longstanding CPython behavior, as far as I can tell. Changing the shutdown behavior seems likely to cause different shutdown-related bugs than what we experience today.

I think we can work around this by encoding more knowledge about generators in gc_free_threading.c. I don't love that, but it seems less risky than messing with the shutdown behavior. Let me know if you want to go that route -- I can help with the gc_free_threading.c changes.

I'd like to investigate fixing up thread state clean up. While it may well risk introducing bugs in the short term, I think to would be better long term. It is hard to optimize anything if we can't trust our supposed invariants.

A quick fix, until we decide on how to handle this long term, would be to traverse the frame list when deleting threads and mark all generators as suspended.

colesbury · 2024-10-22T14:35:20Z

Python/pystate.c

+        do {
+            if (frame->owner == FRAME_OWNED_BY_GENERATOR) {
+                PyGenObject *gen = _PyGen_GetGeneratorFromFrame(frame);
+                gen->gi_frame_state = FRAME_ZOMBIE;
+            }
+            frame = frame->previous;
+        } while (frame != NULL);


It's not sufficient to mark generators as zombies in PyThreadState_Clear(). We call PyThreadState_Clear() one at a time on each non-main thread:

At that point, they are already unlinked from the interpreter's list of PyThreadStates

The PyThreadState_Clear() calls makes escaping calls (Py_CLEAR()) that can trigger the GC

So the GC may still see generators that are marked as "running", but aren't in any accessible PyThreadState's frame stack.

I wrote "At that point, they are already unlinked from the interpreter's list of PyThreadStates", but I don't think that's accurate. Thread states are unlinked in PyThreadState_Delete or PyThreadState_DeleteCurrent, and those happen after PyThreadState_Clear.

Oh... my Oct 22, 2024 comment was in relation to interpreter shutdown, which reverses the typical order. During shutdown thread states are:

Unlinked

Cleared

Deleted

Whereas during normal thread exit the order is:

Cleared

Unlinked

Deleted

Specifically, in _Py_Finalize() we:

First we unlink all the remaining thread states (except the main thread). This is typically for daemon threads, but if you press Ctrl-C it can include non-daemon threads as well:

cpython/Python/pylifecycle.c

Line 2079 in 1150321

PyThreadState *list = _PyThreadState_RemoveExcept(tstate);

Then we call _PyThreadState_DeleteList(), which calls PyThreadState_Clear() on each thread one at a time, which can execute arbitrary code via finalizers and weakrefs.

cpython/Python/pylifecycle.c

Line 2090 in 1150321

_PyThreadState_DeleteList(list, /*is_after_fork=*/0);

cpython/Python/pystate.c

Lines 1904 to 1922 in 1150321

void

_PyThreadState_DeleteList(PyThreadState *list, int is_after_fork)

{

// The world can't be stopped because we PyThreadState_Clear() can

// call destructors.

assert(!_PyRuntime.stoptheworld.world_stopped);

PyThreadState *p, *next;

for (p = list; p; p = next) {

next = p->next;

PyThreadState_Clear(p);

if (is_after_fork) {

free_threadstate((_PyThreadStateImpl *)p);

}

else {

decref_threadstate((_PyThreadStateImpl *)p);

}

}

}

I think things should be okay if we mark the generators as zombies from _PyThreadState_RemoveExcept() as well as from PyThreadState_Clear().

Don't count executing generators and coroutines as referrers in gc.ge…

01bf540

…t_referrers.

markshannon requested review from 1st1, asvetlov, gvanrossum, kumaraditya303 and willingc as code owners October 17, 2024 10:54

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

markshannon added skip news and removed awaiting core review labels Oct 17, 2024

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

[3.14] change in behaviour in gc.get_referrers(some_local) #125603

Open

kumaraditya303 approved these changes Oct 17, 2024

View reviewed changes

bedevere-app bot added the awaiting merge label Oct 17, 2024

colesbury reviewed Oct 17, 2024

View reviewed changes

markshannon changed the title ~~GH-125603: Don't count executing generators and coroutines as referrers in gc.gt_referrers.~~ GH-125603: Don't count executing generators and coroutines as referrers in gc.gc_referrers. Oct 22, 2024

Mark generators left on stack when clearing frame as zombies.

9673b8a

markshannon requested a review from ericsnowcurrently as a code owner October 22, 2024 11:10

colesbury reviewed Oct 22, 2024

View reviewed changes

kumaraditya303 mentioned this pull request Jan 7, 2025

gh-128552: fix refcycles in eager task creation #128553

Merged

graingert mentioned this pull request Jan 25, 2025

simplify test_asyncio.test_taskgroups no_other_refs #129284

Open

musicinmybrain mentioned this pull request Feb 13, 2025

Python 3.14: test_ki_protection_doesnt_leave_cyclic_garbage fails python-trio/trio#3209

Closed

Merge branch 'main' into hide-executing-generator-refs

daa080f

markshannon requested a review from ZeroIntensity as a code owner July 28, 2025 11:45

markshannon added 3 commits July 28, 2025 12:54

Add missing includes

3ba315f

Update test

6b34f05

Keep ruff happy

a59fa5e

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

Uh oh!

GH-125603: Don't count executing generators and coroutines as referrers in gc.gc_referrers. #125640

GH-125603: Don't count executing generators and coroutines as referrers in gc.gc_referrers. #125640

markshannon commented Oct 17, 2024 •

edited by bedevere-app bot

Loading

Uh oh!

kumaraditya303 left a comment

Uh oh!

colesbury Oct 17, 2024

Uh oh!

markshannon Oct 17, 2024

Uh oh!

colesbury Oct 17, 2024

Uh oh!

colesbury Oct 17, 2024

Uh oh!

markshannon Oct 21, 2024

Uh oh!

colesbury Oct 21, 2024

Uh oh!

markshannon Oct 21, 2024

Uh oh!

markshannon Oct 21, 2024

Uh oh!

colesbury Oct 21, 2024

Uh oh!

markshannon Oct 21, 2024

Uh oh!

colesbury Oct 22, 2024

Uh oh!

colesbury Jul 28, 2025

Uh oh!

colesbury Jul 28, 2025

Uh oh!

Uh oh!

	void
	_PyThreadState_DeleteList(PyThreadState *list, int is_after_fork)
	{
	// The world can't be stopped because we PyThreadState_Clear() can
	// call destructors.
	assert(!_PyRuntime.stoptheworld.world_stopped);

	PyThreadState p, next;
	for (p = list; p; p = next) {
	next = p->next;
	PyThreadState_Clear(p);
	if (is_after_fork) {
	free_threadstate((_PyThreadStateImpl *)p);
	}
	else {
	decref_threadstate((_PyThreadStateImpl *)p);
	}
	}
	}

Uh oh!

GH-125603: Don't count executing generators and coroutines as referrers in gc.gc_referrers. #125640

Are you sure you want to change the base?

GH-125603: Don't count executing generators and coroutines as referrers in gc.gc_referrers. #125640

Conversation

markshannon commented Oct 17, 2024 • edited by bedevere-app bot Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

kumaraditya303 left a comment

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!

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!

Uh oh!

markshannon commented Oct 17, 2024 •

edited by bedevere-app bot

Loading