Repository navigation
The garbage collector doesn't take in account that objects of heap allocated types hold a strong reference to their type #84398
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 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']
Activity
vstinner commentedon Apr 7, 2020
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
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 commentedon Apr 7, 2020
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 commentedon Apr 7, 2020
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 commentedon Apr 7, 2020
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 commentedon Apr 7, 2020
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 commentedon Apr 7, 2020
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 commentedon Apr 7, 2020
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 commentedon Apr 7, 2020
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 commentedon Apr 7, 2020
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 commentedon Apr 7, 2020
Checking it more closely, that is incorrect, so we are not missing that visitation :(
serhiy-storchaka commentedon Apr 7, 2020
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.
31 remaining items
encukou commentedon May 22, 2020
Ha! I think I found the issue in PySide now. It's different, but it's still a CPython issue. It's actually a variant of the "defining class" problem from PEP-573.
It's legal to get the value of a slot, using PyType_GetSlot.
It's reasonable to assume that what you put in the tp_traverse slot is what you'll get out using PyType_GetSlot.
If a type's tp_traverse tries to call its superclass's tp_traverse, you can get an infinite loop.
See the situation below or run attached reproducer (build the module, import it and exit
the interpreter).
Getting one's superclass is a bit tricky when dealing entirely with FromSpec
types, but it is definitely *possible*. There's a lot of assumptions the code
can reasonably make. In my reproducer, I stored the superclass in a C static
variable (which is perfectly valid if the module cleanly refuses to work with
subinterpreters or interpreter reload).
Base:
Subclass:
So when the subclass is traversed:
since that's what's recorded in type(self)
Another issue is that
PyType_FromSpec_Allocated types lie about their size:tp->tp_basicsize + Py_SIZE(self) * tp->itemsizeis not the actualallocated amount. This could technically be solved by redefining
__sizeof__,but I'm more worried that it's another edge case of a hack, and There will
probably be other edge cases. Since this is API we want to build on, I'd sleep
easier if it's kept clean.
What would be the downsides of reverting and documenting that tp_traverse
must visit Py_TYPE(self)?
It seems that you visit Py_TYPE(self), it could just end up with unbreakable
reference cycles. We're dealing with modules and classes, which are usually
effectively immortal (and if not, the author probably knows what they're doing).
I don't think it would hurt that much.
pablogsal commentedon May 22, 2020
Not much IMHO, I actually would prefer this solution over any automatic "hacks". The reason is that any hack will be technically violating the contract: if I read the tp_traverse of any of these classes I would say "wait a minute....this is wrong: the class owns a strong reference to its parent so...why is not visiting it?". But the answer would be that we are hacking because we didn't want to force users to migrate once we added the strong reference.
Notice that the root of this problem was bpo-35810. In that issue we added a guide to the docs: https://docs.python.org/3/whatsnew/3.8.html#changes-in-the-c-api. In there it says that you should decref now the parent on tp_dealloc:
but it forgot about that you should also visit the parent in tp_traverse.
So, I propose to do the following:
What do you think?
pablogsal commentedon May 22, 2020
I have been playing with the reproducer and I am a bit confused: The reproducer crashes in the same way even after reverting PR 19414 so it does not seem related to it. This is what I get:
object address : 0x55cf429e4080
object refcount : 4
object type : 0x55cf418e9c60
object type name: type
object repr : <class 'reproducer.Base'>
Fatal Python error: _PyObject_AssertFailed: _PyObject_AssertFailed
Python runtime state: initialized
Current thread 0x00007fc65d2f8740 (most recent call first):
File "<frozen importlib._bootstrap>", line 917 in _find_spec
File "<frozen importlib._bootstrap>", line 982 in _find_and_load_unlocked
File "<frozen importlib._bootstrap>", line 1007 in _find_and_load
File "/home/pablogsal/github/python/master/Lib/re.py", line 124 in <module>
File "<frozen importlib._bootstrap>", line 228 in _call_with_frames_removed
File "<frozen importlib._bootstrap_external>", line 790 in exec_module
File "<frozen importlib._bootstrap>", line 680 in _load_unlocked
File "<frozen importlib._bootstrap>", line 986 in _find_and_load_unlocked
File "<frozen importlib._bootstrap>", line 1007 in _find_and_load
File "/home/pablogsal/github/python/master/Lib/rlcompleter.py", line 142 in attr_matches
File "/home/pablogsal/github/python/master/Lib/rlcompleter.py", line 89 in complete
[1] 162397 abort (core dumped) ./python
This seems to indicate that that reproducer is already implementing incorrectly tp_traverse.
pablogsal commentedon May 22, 2020
Ok, I found the problem. The problem is that the reproduced does not correctly work the reference count of base_class because when construction get tuple of bases:
"PyTuple_SetItem" steals a reference to base_class but "PyModule_AddObject" also does the same, and the refcount is incorrect.
If you add a Py_INCREF before, the crash disappears.
pablogsal commentedon May 23, 2020
To be clear: the other crash is still in the reproduced: the one that Petr describes in his comment. In PR 20264 I have prepared the changes that I proposed previously (including the revert of the hack). Petr, do you mind reviewing it?
encukou commentedon May 23, 2020
Thank you for responding so quickly!
Yes, but I'll only get to it next week.
miss-islington commentedon May 27, 2020
New changeset 1cf15af by Pablo Galindo in branch 'master':
bpo-40217: Ensure Py_VISIT(Py_TYPE(self)) is always called for PyType_FromSpec types (reverts #63613) (GH-20264)
1cf15af
miss-islington commentedon May 28, 2020
New changeset bcbe5c5 by Miss Islington (bot) in branch '3.9':
bpo-40217: Ensure Py_VISIT(Py_TYPE(self)) is always called for PyType_FromSpec types (reverts #63613) (GH-20264)
bcbe5c5