bpo-40217: Ensure Py_VISIT(Py_TYPE(self)) is always called for PyType_FromSpec types (reverts GH-19414)

[pablogsal]

Heap types now always visit the type in tp_traverse. See added docs for details.

This reverts commit 0169d30.

https://bugs.python.org/issue40217

Automerge-Triggered-By: @encukou

[vstinner]

Would it be possible to prepare a PR to explain how to fix the issue?

Also, is there someone interested to fix all C extensions of CPython stdlib for this issue?

[pablogsal]

Would it be possible to prepare a PR to explain how to fix the issue?

Also, is there someone interested to fix all C extensions of CPython stdlib for this issue?

I can do both if we end agreeing that that is the way to go

[encukou]

Are the type->tp_traverse == (traverseproc)foo_traverse checks (and the but not for subclasses remarks) necessary? AFAIK, the check will always be true for calls that Python itself makes.
It might necessary to do the check in the reproducer I posted on the bug, but only because that module's own call to base_traverse.

But it might be that I'm misunderstanding the situation.

[encukou]

Never mind; I'm wrong about that. Sorry.

[encukou]

I'll try going with the idea that each instance of a heap type must Py_VISIT its class, and if it calls another heap type's tp_traverse, it delegates the responsibility. Then, it should be possible to remove the if from simple tp_traverse implementations.
I need to run refleak checks to verify things, so it'll take some time. Let's not duplicate the work.

[pablogsal]

@encukou Could you elaborate a bit more the idea you are proposing? I would love to eliminate the checks for subclasses bit sadly the subclass traverse is already visiting the type itself when traversing the hierarchy.

[encukou]

The Python 3.8 behavior is buggy. If a custom tp_traverse visits Py_TYPE(self), its subclasses will crash; if it doesn't, the type may become immortal. I expect that all extension types that use this now either have the latter bug, or are relying on buggy, undocumented implementation details.

I don't like the "Ensure that all custom tp_traverse functions of heap-allocated types visit the object parent but not for subclasses" guideline: it is difficult to understand, and more importantly, subtype_traverse itself doesn't follow it.

So, I'd like to acknowledge that Python 3.8 has the bug, set up a cleaner rule on what tp_traverse should do, and fix subtype_traverse to follow it. Specifically: The traversal function of a heap type must either visit Py_TYPE(self), or delegate this responsibility by calling tp_traverse of another heap type (such as a superclass).

I'll make a PR out of that shortly.

[encukou]

See #20433

[pablogsal]

Gotcha! I very much agree with your analysis. Do you plant to backport this?

Feel free to push to this PR of you wish

[encukou]

I don't think it's worth it to backport, unfortunately.
It's dangerous to visit Py_TYPE(self) in unpatched Python 3.8, as you found out: you need the if (type->tp_traverse == (traverseproc)foo_traverse) to work around the bug in subtype_traverse, otherwise you get crashes.

Not visiting Py_TYPE(self) is not critical: it results in heap types with custom tp_traverse being immortal if part of a reference loop. That's a pretty exotic scenario. Note that it has not been reported against 3.8.

IMO, it is more important to be consistent across 3.8.x than to avoid the leak. If backported to 3.8.4, we get an unfortunate case:

• someone adds the Py_TYPE(self) visit (without if) and tests on Python 3.8.4 successfully
• the extension then crashes on Python 3.8.3 (which has the same ABI)

[miss-islington]

@pablogsal: Status check is done, and it's a success ✅ .

[miss-islington]

Thanks @pablogsal for the PR 🌮🎉.. I'm working now to backport this PR to: 3.9.
🐍🍒⛏🤖

[bedevere-bot]

GH-20490 is a backport of this pull request to the 3.9 branch.

[encukou]

Correction: I do want to backport to 3.9. That's probably what you were asking about, right?
For 3.8 it doesn't make sense, but the docs are written with 3.9 in mind.