bpo-44263: 8000 Py_TPFLAGS_HAVE_GC requires tp_traverse #26463
Conversation
|
This change breaks C extensions which define types with Py_TPFLAGS_HAVE_GC but without tp_traverse, even if instances of the type are never tracked by the GC. Such extensions worked properly previously. Maybe the check could be restricted to heap types? If we don't want to break the backward compatibility, we should check in PyObject_GC_Track() if Many instances are created by PyType_GenericAlloc() which calls _PyObject_GC_TRACK(). I tried to modify _PyObject_GC_TRACK() to ensure that it's possible to traverse the instance, as I did in PyObject_GC_Track(), but I got a lot of issues. Many types call _PyObject_GC_TRACK() too early, and it's really difficult to change that (it would require a lot of work). |
|
As I explained at https://bugs.python.org/issue44263#msg394799, this PR cannot be merged yet since _testcapi and _decimal don't respect the GC protocol. I chose to create a separated PR to fix them, so it can be backported (and review should be easier): #26464 |
My latest attempt in 2020: https://bugs.python.org/issue40142 |
| (Contributed by Dong-hee Na and Serhiy Storchaka in :issue:`44235`.) |
There was a problem hiding this comment.
Why this change? :)
There was a problem hiding this comment.
My text editor (vim) likes to add newline at the end of a file.
|
@vstinner is the issue number correct? |
The PyType_Ready() function now raises an error if a type is defined with the Py_TPFLAGS_HAVE_GC flag set but has no traverse function (PyTypeObject.tp_traverse).
I consider that it's to ok reuse https://bugs.python.org/issue44263 My change implements a runtime check for 8b55bc3 "... it must implement the GC protocol itself by at least including a slot". |
|
PR rebased on top on the merged PR #26464 fix. |
🤷♂️ I don't know about that, https://bugs.python.org/issue44263 is a bout a documentation change and this is a breaking change in the C-API, which probably requires its own issue. But I don't feel strongly so you don't need to change anything. I just want to note that I find it a bit odd. |
|
For the risk of breaking C extensions with this PR: I documented the change in What's New in Python 3.11. It's the bare minimum change that I can do. @pablogsal: Apart of the bpo number, are you ok with this change? A GC type with NULL traverse function is likely to crash, but not always, as we saw with the broken _ssl.SSLError exception ;-) This change makes the code more deterministic, it always fail (but don't crash) ;-) |
Hm. Ptr's pep-652 have redefined the lifetime of stable ABI, so it's not a big probleam, right? |
This doesn't change the ABI, it just makes a bug break early :) |
OK. It's make sense :) |
|
Ok, I merged my PR. If it breaks too many C extensions, we can revert it. I hope that it will detect more real bugs than it breaks C extensions for no good reasons :-D |
The PyType_Ready() function now raises an error if a type is defined
with the Py_TPFLAGS_HAVE_GC flag set but has no traverse function
(PyTypeObject.tp_traverse).
https://bugs.python.org/issue44263