gh-123241: Don't modify ref count during visitation

[DinoV]

traverse functions shouldn't modify the ref count during visitation otherwise they'll cause issues in the free threaded GC. CType_Type_traverse uses _PyStgInfo_FromType_NoState to get back to the state object but this causes an incref to happen via PyType_GetBaseByToken.

Instead of using PyType_GetBaseByToken this just walks to find the CType's solid base. Because PyCType_Type adds fields and immediately descends from PyType_Type it will always be the base immediately before PyType_Type.

• Issue: GC in free-threaded build has problems with Py_INCREF()/Py_DECREF() in tp_traverse handlers #123241

[encukou]

traverse functions shouldn't modify the ref count during visitation otherwise they'll cause issues in the free threaded GC.

Is this limitation documented? That sounds like it should be mentioned in the docs, and a free-threading porting guide.

Because PyCType_Type adds fields and immediately descends from PyType_Type it will always be the base immediately before PyType_Type.

There's an assumption I'd like to relax eventually :)
Could we add a _PyType_GetBaseByToken_Borrow instead?
If not, please add an assertion that the token matches.

[colesbury]

Was this found from a crash or by inspecting the code? Either way, I think it's worth fixing, but my understanding was that the problematic pattern involved incref/decref around the visit, i.e. something like:

Py_INCREF(obj);
Py_VISIT(obj);
Py_DECREF(obj);

While this code was more like:

Py_INCREF(obj);
Py_DECREF(obj);
Py_VISIT(obj);

I think we can make the free threaded GC less susceptible to this problem, but we should also document the things you shouldn't do in a tp_traverse handler.

[colesbury]

We shouldn't create exceptions (or any other object) during tp_traverse handlers either. I think this should be pulled up to the tp_dealloc / tp_clear if we want to print unraisable exceptions there.

[mpage]

Was this found from a crash or by inspecting the code?

I found this from a crash that occurred while porting the parallel GC in cinder (it can call the same traverse function from multiple threads) to 3.14.

[DinoV]

There's an assumption I'd like to relax eventually :) Could we add a _PyType_GetBaseByToken_Borrow instead? If not, please add an assertion that the token matches.

Sounds good, I've gone ahead and added the Borrow version and switched to using it instead.

[encukou]

Thanks! Looks great; I left some nitpicks in DinoV#2

[miss-islington-app]

Thanks @DinoV for the PR, and @encukou for merging it 🌮🎉.. I'm working now to backport this PR to: 3.14.
🐍🍒⛏🤖

[miss-islington-app]

Sorry, @DinoV and @encukou, I could not cleanly backport this to 3.14 due to a conflict.
Please backport using cherry_picker on command line.

cherry_picker da8199f8842c2830fa4015138725849863523de4 3.14

[encukou]

Thank you!

[bedevere-app]

GH-142567 is a backport of this pull request to the 3.14 branch.