|
🤖 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. |
Sorry, something went wrong.
There was a problem hiding this comment.
LGTM
Sorry, something went wrong.
There was a problem hiding this comment.
All ref leaks are gone!
Thanks for the fix.
Sorry, something went wrong.
|
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). |
Sorry, something went wrong.
|
🤖 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. |
Sorry, something went wrong.
There was a problem hiding this comment.
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).
Sorry, something went wrong.
|
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, Lines 6670 to 6678 in 0bb61dd 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. |
Sorry, something went wrong.
|
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. |
Sorry, something went wrong.
|
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. |
Sorry, something went wrong.
Uh oh!
There was an error while loading. Please reload this page.
Sorry, something went wrong.
Uh oh!
There was an error while loading. Please reload this page.