GH-104510: Fix refleaks in _io base types

[kumaraditya303]

• Issue: Ref leaks introduced by _io isolation (gh-101948) #104510

[bedevere-bot]

🤖 New build scheduled with the buildbot fleet by @kumaraditya303 for commit 5ee2358 🤖

If you want to schedule another build, you need to add the 🔨 test-with-refleak-buildbots label again.

[sunmy2019]

LGTM

[Eclips4]

All ref leaks are gone!
Thanks for the fix.

[erlend-aasland]

The reason why this fix works is not obvious. Can you add a clarifying comment, alternatively expand on the C API docs for tp_traverse?

Note to self: we also need to document the GC peculiarities of heap-type-inherits-from-static-type (for example defdict in the _collections extension module).

[bedevere-bot]

🤖 New build scheduled with the buildbot fleet by @kumaraditya303 for commit 78c12ff 🤖

If you want to schedule another build, you need to add the 🔨 test-with-refleak-buildbots label again.

[erlend-aasland]

AFAICS, this is the correct solution.

We still need to write a sentence or two in the docs about our recent GC adventures (_io, _collections).

[sunmy2019]

The heap type still has GC but it now inherits traverse and clear slots from its base because Py_TPFLAGS_HAVE_GC isn't set. See https://docs.python.org/3.12/c-api/gcsupport.html

The doc's a bit unclear here.

When calling PyType_Ready() or some of the APIs that indirectly call it like PyType_FromSpecWithBases() or PyType_FromSpec() the interpreter will automatically populate the tp_flags, tp_traverse and tp_clear fields if the type inherits from a class that implements the garbage collector protocol and the child class does not include the Py_TPFLAGS_HAVE_GC flag.

From the code, we see,

cpython/Objects/typeobject.c

Lines 6670 to 6678 in 0bb61dd

	if (!(type->tp_flags & Py_TPFLAGS_HAVE_GC) &&
	(base->tp_flags & Py_TPFLAGS_HAVE_GC) &&
	(!type->tp_traverse && !type->tp_clear)) {
	type->tp_flags |= Py_TPFLAGS_HAVE_GC;
	if (type->tp_traverse == NULL)
	type->tp_traverse = base->tp_traverse;
	if (type->tp_clear == NULL)
	type->tp_clear = base->tp_clear;
	}

To get the tp_traverse and tp_clear inherited, the child type must have them both NULL.

This is against the doc. Also, the comparisons in lines 6674 and 6676 are unnecessary.

It was a 22-yr old code 13d52f0. I guess the author (@gvanrossum) was trying to write

-      (!type->tp_traverse && !type->tp_clear)) { 
+      (!type->tp_traverse || !type->tp_clear)) {  

Don't know what to do. Maybe we should only update the doc.

[kumaraditya303]

Don't know what to do. Maybe we should only update the doc.

The best we can do is to fix docs, the behavior here is ancient.

[vstinner]

Next time, it would be nice to elaborate the rationale for the fix in the commit, rather than just saying "fix" without any commit message ;-) It's really hard to handle the GC correctly with heap types. Better documentation is required.

The _io module is one of the few C extensions which use class inheritance with heap types implemented in C.

[vstinner]

cc @pablogsal: GC issues are hard :-(