Repository navigation
Object Initialization does not incref Heap-allocated Types #79991
Description
Note: these values reflect the state of the issue at the time it was migrated and might not reflect the current state.
Show more details
GitHub fields:
assignee = None
closed_at = <Date 2019-05-22.11:50:41.239>
created_at = <Date 2019-01-23.20:35:52.918>
labels = ['extension-modules', 'interpreter-core', 'type-bug', '3.8']
title = 'Object Initialization does not incref Heap-allocated Types'
updated_at = <Date 2020-02-24.07:27:49.502>
user = 'https://github.com/eduardo-elizondo'bugs.python.org fields:
activity = <Date 2020-02-24.07:27:49.502>
actor = 'Christian.Tismer'
assignee = 'none'
closed = True
closed_date = <Date 2019-05-22.11:50:41.239>
closer = 'petr.viktorin'
components = ['Extension Modules', 'Interpreter Core']
creation = <Date 2019-01-23.20:35:52.918>
creator = 'eelizondo'
dependencies = []
files = []
hgrepos = []
issue_num = 35810
keywords = ['patch', 'patch', 'patch']
message_count = 23.0
messages = ['334271', '334347', '334523', '334585', '334818', '335343', '335348', '335367', '335368', '335370', '335419', '335555', '335592', '335695', '336052', '336247', '336250', '336286', '336289', '336515', '338955', '362527', '362571']
nosy_count = 9.0
nosy_names = ['nascheme', 'ncoghlan', 'scoder', 'vstinner', 'petr.viktorin', 'eric.snow', 'Christian.Tismer', 'eelizondo', 'Ondrej Jakubcik']
pr_nums = ['11661', '11661', '11661', '16071']
priority = 'normal'
resolution = 'fixed'
stage = 'resolved'
status = 'closed'
superseder = None
type = 'behavior'
url = 'https://bugs.python.org/issue35810'
versions = ['Python 3.8']
Activity
eduardo-elizondo commentedon Jan 23, 2019
Heap-allocated Types initializing instances through
PyObject_{,GC}_New{Var}will NOT not have their refcnt increased. This was totally fine under the assumption that static types are immortal. However, heap-allocated types MUST participate in refcounting. Furthermore, their deallocation routine should also make sure to decrease their refcnt to provide the incref/decref pair.scoder commentedon Jan 25, 2019
It seems right that a heap allocate object owns a reference to its (non-static) type. But the mere fact that you had to adapt stdlib code makes it obvious that this will also break existing user code out there. And such breakage is very likely to remain undetected for a while, since leaking types is unlikely to have a big enough memory impact in most application to be easily detected.
This is a tough call. Any ideas for reducing the chance of breakage or increasing the chance of detecting it in user code would help.
encukou commentedon Jan 29, 2019
I would very much like to see this fixed.
Yes, it can make some extension types immortal, but I believe those types were relying on buggy behavior, and need to be fixed to prevent memory leaks.
I think this is a big enough change to be mentioned in What’s New.
I'd recommend that all C extensions have leak detection as part of their test suite. I don't think we can do much better :(
Indeed, it's not a big enough problem in most applications.
eduardo-elizondo commentedon Jan 30, 2019
Hi Petr,
Please take a look at the Github PR. What do you think that's missing to move this forward? I'd be more than happy to add more documentation/testing to it. Let me know what you think
encukou commentedon Feb 4, 2019
I'll allocate time this week for the review.
scoder commentedon Feb 12, 2019
Victor asked me for a review, so, well, what should I say? The intention seems right, and the patch also looks good to me.
From the top of my head, I wouldn't know any problems this would produce with Cython specifically, although it's worth testing. If we find anything, then it's hopefully easy to adapt to in a point release, which Cython users can then build their code with to support Py3.8+. That's the way it usually works with Cython.
The main problem I see is that while this change may crash in some cases (the lucky cases that are easy to detect), it will leak references in others, which can be really difficult to detect. My own biased little gut feeling still wants me to believe that the impact of this could be somewhat low. Why? Well, how many heap-allocated types with a custom "tp_dealloc()" do you expect there to be? My feeling is that most existing code still uses statically allocated types for that. CPython has a couple of examples (that the PR adapts), but IIRC, that's mostly because some core devs wanted to test-ride parts of the newer type creation C-API in the standard library (a perfectly valid reason, but that also makes it a bad example). From the little valley that I sit in, I don't see a large bunch of other usages of that API out in the wild. That doesn't mean they are not there, and there might well be some large projects that could be bitten by this change. But I'm sure it's not the majority.
So, on the one hand, any breaking change to the C-API may make users end up with little maintained projects that they depend on and that break in Py3.y and later without anyone having access to the PyPI project account to push a fix release. Very annoying situation.
On the other hand, a breaking C-API change is not the only problematic case, and people have to deal with similar situations anyway. CPython changes are really just one of many, many ways to render your code unusable.
I would suggest clear, open communication of this. It's solving a bug. It makes CPython safer. It's not difficult to adapt your code, once you know it's affected. The usual PY_VERSION_HEX guard will do it for you. I think we should risk it.
vstinner commentedon Feb 12, 2019
It would be nice to give an example of code in the Porting guide (What's new in Python 3.8?).
eduardo-elizondo commentedon Feb 12, 2019
Thanks for the thorough feedback Stefan!
I would like to just add an extra thing to everything you already mentioned:
This change will NEVER cause a crash. The change that we are introducing is an incref to a type object (no decrefs). Thus, there are two-ish scenarios:
2.1) The type is recounted (automatically): Achieved through the generic allocation which already increfs the type. Given that this refactors that incref, then this behavior should stay exactly the same.
2.2) The type is refcounted (manually): Achieved by not using the generic allocation and instead using
PyObject_{,GC}_New{Var}. To properly refcount this type, a manual incref is required after object instantiation. Usually, I've seen this pattern in very carefully engineered types where a NULL is jammed intotp_newto make it into a non-instantiable type. Examples of this are Modules/_tkinter.c and Modules/_curses_panel.c. Anyways, adding this incref will leak this type, but will not cause a crash.eduardo-elizondo commentedon Feb 12, 2019
Now, with that in mind - can you guys point me to the right thing to do now - how can we move this forward? :)
Let me know what you think!
scoder commentedon Feb 12, 2019
Adding Christian Tismer to the nosy list since he might be able to elaborate on the impact on PySide (which IIRC uses the stable ABI, and thus, heap types).
3 remaining items
eduardo-elizondo commentedon Feb 14, 2019
Looking forward to seeing this through!
ctismer commentedon Feb 15, 2019
Thanks for including me here!
We have indeed converted everything to new style types
but saw no refcount problems, yet. This will probably
come after the patch. We will add an issue to the tracker
for Python 3.8.
ncoghlan commentedon Feb 16, 2019
Having types created through the stable ABI potentially be deallocated when their instances are deallocated is indeed a major problem, and fixing that seems worth the risk of some types that were designed to handle that become immortal.
eduardo-elizondo commentedon Feb 20, 2019
Bump to get a review on the PR: #11661.
I believe all comments have now been addressed as well as adding a porting to 3.8 guide.
nascheme commentedon Feb 21, 2019
Hello Eddie,
Thank you for putting what looks to be significant effort into this PR. It would be great if we can get this fixed. There is a real issue about breaking 3rd party extensions. So, we want to proceed with care.
I wonder, if we are going to break extensions already, could we just remove the whole concept of heap allocated types? If you look through the CPython source code, I think you will find a lot of tricky code that deals with the Py_TPFLAGS_HEAPTYPE case. If we could remove heap types, we could remove all those cases. That would give some performance improvement but more importantly would simplify the implementation.
If PyType_FromSpec() is now working correctly, could we just move everything that the currently a heap type to use that? Obviously we have to give 3rd party extensions a lot of time to get themselves updated. Maybe give a deprecation warning if Py_TPFLAGS_HEAPTYPE is used. You could have a configuration option for Python that enables or disables the Py_TPFLAGS_HEAPTYPE support. Once we think extensions have been given enough time to update themselves, we can remove Py_TPFLAGS_HEAPTYPE.
Some other possible advantages of getting rid of heap types:
GC objects will always have the GC header allocated (because CPython controls the allocation of the chunk of memory for the type)
might be possible to eliminate GC headers and use bitmaps. I have been experimenting with the idea but it seems to require that we don't use heap types. Initially I was interested in the bitmap idea because of memory savings. After more tinkering, I think the big win will be in eliminating linked-list traversals. On modern CPUs, that's slow and iterating over a bitmap should be much faster.
I suspect heap types are difficult to support for PyPy. I haven't looked into that but it seems tricky when you have non-refcounting GC
type_is_gc() is ugly and would go away. Based on my profiling, PyObject_IS_GC() is pretty expensive. A lot of types have the tp_is_gc slot set (more than you would expect).
In the very long term, using PyType_FromSpec() could give us the freedom to change the structure layout of types. I don't have any specific ideas about that but it seems like a better design.
nascheme commentedon Feb 21, 2019
Sorry, morning coffee didn't kick in yet I guess. ;-) My actual wish is to make all types heap allocated and eliminate the statically allocated ones. So, Py_TPFLAGS_HEAPTYPE would be set on all types in that world. That is a gigantic task, affecting near every Python extension type. Too huge for even a nutty person like me to imagine doing in the near term. So, sorry for potentially derailing discussion here.
I agree with comments made by Stefan Behnel and Petr Viktorin. There is a small risk to cause problems (i.e. serious memory leaks in a previously working program). However, as Petr says, the extension in that case is broken and it is not hard to fix. Eddie has provided examples for what changes are needed.
I think if we properly communicate the change then it is okay to merge the PR.
ctismer commentedon Feb 22, 2019
Neil, that is the absolute super-move!
When all types are heap types, then I have no longer the problem
that I cannot get at slots from builtin types, with all are static.
I am very much for that change, because then I can make my stable
ABI implementation of PySide much cleaner.
encukou commentedon Feb 22, 2019
Changing all types to heap types is definitely a gigantic task. First let's make heap types more usable and bug-free, and then it will be easier :)
(I do have an agenda here: improving heap types usable is also yak-shaving* for making modules play nice with subinterpreters, like in PEPs 489 & 573)
eduardo-elizondo commentedon Feb 25, 2019
I do have plans to start migrating more and more CPython modules to use heap allocated types. For example I have posixmodule up in the queue (PR10854) and a local change with all of _io migrated. I'm only blocked by having correct refcnts in these types.
So, yes, let's work towards migrating all the static types to heap-allocated types! I have the time and energy to embark on this huge task so you'll see more and more of these PRs from me in the future. :)
In that way, I agree with Petr, let's start by fixing the core issues first.
With all of that in mind, it seems to me that we are all agree on the current solution. Let's try to push this forward and merge the PR.
encukou commentedon Mar 27, 2019
New changeset 364f0b0 by Petr Viktorin (Eddie Elizondo) in branch 'master':
bpo-35810: Incref heap-allocated types in PyObject_Init (GH-11661)
364f0b0
OndrejJakubcik commentedon Feb 23, 2020
How does this change affect stable ABI? Is it necessary to change the logic in modules that use only the Py_LIMITED_API?
ctismer commentedon Feb 24, 2020
If you use heap types, you need to adjust refcounts beginning
with Python 3.8 . And since the Py_LIMITED_API uses heap types
without hiding these refcount changes by an API call,
this breaks the stable API.
But it is easily fixable by a runtime version check, or you start
over with Python 3.8 as lowest version <wink>.
PyCLIF-C-API refcount bug fixes and `tp_init_with_safety_checks` chan…