Object Initialization does not incref Heap-allocated Types

[eduardo-elizondo]

BPO	35810
Nosy	@nascheme, @ncoghlan, @scoder, @vstinner, @encukou, @ericsnowcurrently, @ctismer, @eduardo-elizondo
PRs	bpo-35810: Incref heap-allocated types in PyObject_Init #11661 bpo-35810: Incref heap-allocated types in PyObject_Init #11661 bpo-35810: Incref heap-allocated types in PyObject_Init #11661 bpo-38142: Updated _hashopenssl.c to be PEP 384 compliant #16071

^{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']

[eduardo-elizondo]

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]

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]

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.

Any ideas for reducing the chance of breakage or increasing the chance of detecting it in user code would help.

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 :(

leaking types is unlikely to have a big enough memory impact in most application to be easily detected.

Indeed, it's not a big enough problem in most applications.

[eduardo-elizondo]

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]

I'll allocate time this week for the review.

[scoder]

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]

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.

It would be nice to give an example of code in the Porting guide (What's new in Python 3.8?).

[eduardo-elizondo]

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:

1. The type object is immortal: This means that the type does not incref/decref its refcount automatically on instance creation. Adding this incref will not affect the fact that it's already immortal. An example of this is structsequences. The change that I added in the PR is to convert it to an refcounted type (instead of immortal).

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 into tp_new to 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]

Now, with that in mind - can you guys point me to the right thing to do now - how can we move this forward? :)

• Should I write something up in python-dev/Discourse?
• Do I need to update the PY_VERSION_HEX?
• Do I need to write an entry in the Porting guide?

Let me know what you think!

[scoder]

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).

[vstinner]

• Should I write something up in python-dev/Discourse?

Please open a thread on python-dev. The purpose is not really to ask if it's worth it, but more to communicate properly on backward incompatible changes.

• Do I need to update the PY_VERSION_HEX?

No. The release manager is responsible for that.

• Do I need to write an entry in the Porting guide?

Yes. You should add a new "Changes in the C API" section to:
https://docs.python.org/dev/whatsnew/3.8.html#porting-to-python-3-8

This is why you can show an example of dealloc using #ifdef with PY_VERSION_HEX.

Example in 3.7 doc:
https://docs.python.org/dev/whatsnew/3.7.html#changes-in-the-c-api

[eduardo-elizondo]

Please open a thread on python-dev
Done! https://mail.python.org/pipermail/python-dev/2019-February/156322.html

Yes. You should add a new "Changes in the C API"
Done as well, I also included examples for the scenarios that will need fixing to avoid having immortal types: #11661

Looking forward to seeing this through!

[ctismer]

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]

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]

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]

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]

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]

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]

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)

• https://en.wiktionary.org/wiki/yak_shaving

[eduardo-elizondo]

could we just remove the whole concept of heap allocated types?

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. :)

First let's make heap types more usable and bug-free, and then it will be easier

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]

New changeset 364f0b0 by Petr Viktorin (Eddie Elizondo) in branch 'master':
bpo-35810: Incref heap-allocated types in PyObject_Init (GH-11661)
364f0b0

[OndrejJakubcik]

How does this change affect stable ABI? Is it necessary to change the logic in modules that use only the Py_LIMITED_API?

[ctismer]

How does this change affect stable ABI? Is it necessary to change the
logic in modules that use only the Py_LIMITED_API?

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>.