The garbage collector doesn't take in account that objects of heap allocated types hold a strong reference to their type

[vstinner]

BPO	40217
Nosy	@tim-one, @vstinner, @encukou, @ambv, @serhiy-storchaka, @corona10, @pablogsal, @miss-islington, @shihai1991
PRs	bpo-40217: Ensure Py_VISIT(Py_TYPE(self)) is always called for PyType_FromSpec types #19414 bpo-40217: Clean code in PyType_FromSpec_Alloc and add NEWS entry #19733 bpo-40217: Ensure Py_VISIT(Py_TYPE(self)) is always called for PyType_FromSpec types (reverts GH-19414) #20264 bpo-40217: Make heap types always visit Py_TYPE(self) from tp_traverse #20433 [3.9] bpo-40217: Ensure Py_VISIT(Py_TYPE(self)) is always called for PyType_FromSpec types (reverts GH-19414) (GH-20264) #20490
Files	reproducer.c

^{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 2020-06-01.07:35:10.755>
created_at = <Date 2020-04-07.16:23:41.763>
labels = ['interpreter-core', '3.9', 'release-blocker']
title = "The garbage collector doesn't take in account that objects of heap allocated types hold a strong reference to their type"
updated_at = <Date 2020-06-01.07:35:10.755>
user = 'https://github.com/vstinner'

bugs.python.org fields:

activity = <Date 2020-06-01.07:35:10.755>
actor = 'petr.viktorin'
assignee = 'none'
closed = True
closed_date = <Date 2020-06-01.07:35:10.755>
closer = 'petr.viktorin'
components = ['Interpreter Core']
creation = <Date 2020-04-07.16:23:41.763>
creator = 'vstinner'
dependencies = []
files = ['49182']
hgrepos = []
issue_num = 40217
keywords = ['patch']
message_count = 44.0
messages = ['365911', '365918', '365921', '365922', '365923', '365924', '365925', '365926', '365927', '365928', '365929', '365930', '365931', '365932', '365933', '365963', '365965', '367119', '367120', '367122', '367124', '367125', '367150', '367270', '367412', '367413', '367416', '367424', '367425', '367681', '368813', '369460', '369462', '369464', '369532', '369599', '369632', '369636', '369668', '369669', '369675', '369713', '370057', '370225']
nosy_count = 9.0
nosy_names = ['tim.peters', 'vstinner', 'petr.viktorin', 'lukasz.langa', 'serhiy.storchaka', 'corona10', 'pablogsal', 'miss-islington', 'shihai1991']
pr_nums = ['19414', '19733', '20264', '20433', '20490']
priority = 'release blocker'
resolution = 'fixed'
stage = 'resolved'
status = 'closed'
superseder = None
type = None
url = 'https://bugs.python.org/issue40217'
versions = ['Python 3.9']

[vstinner]

The bpo-35810 modified the object allocate to hold a *strong* reference to the type in PyObject.ob_type, whereas PyObject.ob_type is a *borrowed* references if the type is statically allocated.

commit 364f0b0
Author: Eddie Elizondo <eduardo.elizondorueda@gmail.com>
Date: Wed Mar 27 07:52:18 2019 -0400

bpo-35810: Incref heap-allocated types in PyObject_Init (GH-11661)

* Incref heap-allocated types in PyObject_Init
* Add documentation and porting notes to What's New

The problem is now in some corner cases, the GC fails to visit all referrer of a type and so considers that the type is still alive.

bpo-40149 is a concrete example of such bug.

I propose to modify the GC to take bpo-35810 in account.

... or maybe I just misunderstood bpo-40149 bug :-)

[pablogsal]

I propose to modify the GC to take bpo-35810 in account.

What? The GC is agnostic of what it receives. It works with objects in general that implement the gc support, but it does not care about what those objects are. The only specal case are weakreferences and because those have inherit GC semantics.

I am not sure about what you are proposing, could you elaborate?

[pablogsal]

Also, heap types (created with type_new()) are already taking into account the type when visiting references:

https://github.com/python/cpython/blob/master/Objects/typeobject.c#L1111

[tim-one]

If object A owns a strong reference to object B, and A participates in cyclic gc, and B may be part of a cycle, then it's necessary and sufficient that A's type's tp_traverse implementation invoke Py_VISIT() passing A's pointer to B.

It would be a Really Bad Idea to add special cases to the gc module to spare some specific type(s) from following that (currently) utterly uniform rule.

[serhiy-storchaka]

It would be inconvenient to require adding Py_VISIT(Py_TYPE(self)) in all tp_visit implementations of heap allocated types (including third-party extensions). Since tp_visit is GC specific thing, I think it wold be more convenient make a special case for object types.

[pablogsal]

>> Since tp_visit is GC specific thing, I think it wold be more convenient make a special case for object types.

I don't think that follows: The gc defers the logic of what and what should not be visited to the tp_traverse implementation of every object. The GC must be agnostic of the types it receives and heap types here are not special.

--

Also, if we make an exception in the GC for this special case, all tp_traverse of all those functions will be incorrect. Someone reading those tp_traverse can say "Oh, why these functions do not visit the type if they own s reference to it, this looks incorrect" and then we need to explain that that is an exception in the GC just because we were lazy to implement them correctly.

[serhiy-storchaka]

Interesting, this issue may be related to bpo-24379. The problem with the proposed implementation of subscript was that it created a reference loop, and not all links in this loop were visible by GC.

[pablogsal]

Also, as I mentioned, you don't need to modify all objects tp_traverse, only it's type.tp_traverse slot. For instance, all python objects know how to traverse stuff because they share the same tp_traverse:

https://github.com/python/cpython/blob/master/Objects/typeobject.c#L1082

So unless I am missing something, if you want to affect all heap types you just need to modify one tp_traverse in one place: the superclass.

[pablogsal]

Hummm, I think we may just be missing a Py_VISIT(Py_TYPE(self))here:

https://github.com/python/cpython/blob/master/Objects/typeobject.c#L3562

A class owns references to its type (type) or another metaclass

Tim, could you confirm that that is the case?

[pablogsal]

Hummm, I think we may just be missing a Py_VISIT(Py_TYPE(self))here:

Checking it more closely, that is incorrect, so we are not missing that visitation :(

[serhiy-storchaka]

Recently many static allocated types were converted to heap allocated types (using PyType_FromSpec). Now you need to add Py_VISIT(Py_TYPE(self)) in all corresponding tp_visit implementations.

And since even official example for heap allocated types do not contain it (and it was not needed before 3.9), I am sure that all existing code do not contain it.

We cannot change all user code, so we should change the interpreter code so that it will work correctly with existing user code.