GeezerVison is showing github.com — tp_dealloc trashcan shouldn't be called for subclasses · Issue #80164 · python/cpython
tp_dealloc trashcan shouldn't be called for subclasses #80164Labels 3.8 (EOL)end of lifeend of lifeinterpreter-core(Objects, Python, Grammar, and Parser dirs)(Objects, Python, Grammar, and Parser dirs)type-bugAn unexpected behavior, bug, or errorAn unexpected behavior, bug, or error 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 = None
created_at = <Date 2019-02-13.11:02:31.402>
labels = ['interpreter-core', 'type-bug', '3.8']
title = "tp_dealloc trashcan shouldn't be called for subclasses"
updated_at = <Date 2019-05-15.14:34:02.694>
user = 'https://github.com/jdemeyer' bugs.python.org fields:
activity = <Date 2019-05-15.14:34:02.694>
actor = 'ned.deily'
assignee = 'none'
closed = False
closed_date = None
closer = None
components = ['Interpreter Core']
creation = <Date 2019-02-13.11:02:31.402>
creator = 'jdemeyer'
dependencies = []
files = []
hgrepos = []
issue_num = 35983
keywords = ['patch']
message_count = 19.0
messages = ['335405', '335406', '335440', '335453', '335455', '335463', '335476', '335513', '338976', '338981', '338982', '339017', '339021', '339022', '339023', '339119', '339519', '339609', '342080']
nosy_count = 6.0
nosy_names = ['pitrou', 'scoder', 'eric.snow', 'serhiy.storchaka', 'jdemeyer', 'matrixise']
pr_nums = ['11841', '12607', '12699', '12730']
priority = 'normal'
resolution = None
stage = 'patch review'
status = 'open'
superseder = None
type = 'behavior'
url = 'https://bugs.python.org/issue35983'
versions = ['Python 3.8'] Activityjdemeyer commented on Feb 13, 2019 ContributorAuthor More actions When designing an extension type subclassing an existing type, it makes sense to call the tp_dealloc of the base class from the tp_dealloc of the subclass.
Now suppose that I'm subclassing "list" which uses the trashcan mechanism. Then it can happen that the tp_dealloc of list puts this object in the trashcan, even though the tp_dealloc of the subclass has already been called. Then the tp_dealloc of the subclass may be called multiple times, which is unsafe (tp_dealloc is expected to be called exactly once).
To solve this, the trashcan mechanism should be disabled when tp_dealloc is called from a subclass.
Interestingly, subtype_dealloc also solves this in a much more involved way (see the "Q. Why the bizarre (net-zero) manipulation of _PyRuntime.trash_delete_nesting around the trashcan macros?") in
Objects/typeobject.c matrixise commented on Feb 13, 2019 Jeroen, could you share your example? I am learning the C-API of Python and this example could be interesting. jdemeyer commented on Feb 13, 2019 ContributorAuthor More actions The problem is easily reproduced with Cython:
cdef class List(list):
cdef int deallocated
def \_\_dealloc__(self):
if self.deallocated:
print("Deallocated twice!")
self.deallocated = 1
L = None
for i in range(10**4):
L = List((L,))
del L ericsnowcurrently commented on Feb 13, 2019 FWIW, subclassing builtin types is a relatively new thing. There are still a number of lingering (older) implementation details throughout CPython that were written assuming no subclassing. I'd guess that this is one of them. pitrou commented on Feb 13, 2019 By "relatively new thing", you mean less than 20 years old? :-) ericsnowcurrently commented on Feb 13, 2019 On Wed, Feb 13, 2019 at 8:42 AM Antoine Pitrou <[email protected]> wrote:
Antoine Pitrou <[email protected]> added the comment:
By "relatively new thing", you mean less than 20 years old? :-)
Yeah, looks like it was in the 2.2 release (Dec 2001) for PEP-253.
Anyway, I know of several core devs who are still opposed to
subclassing builtin types. :) jdemeyer commented on Feb 13, 2019 ContributorAuthor More actions NOTE: also OrderedDict currently uses trashcan hacking to work around this problem:
/* Call the base tp_dealloc(). Since it too uses the trashcan mechanism,
* temporarily decrement trash_delete_nesting to prevent triggering it
* and putting the partially deallocated object on the trashcan's
* to-be-deleted-later list.
*/
--tstate->trash_delete_nesting;
assert(_tstate->trash_delete_nesting < PyTrash_UNWIND_LEVEL);
PyDict_Type.tp_dealloc((PyObject *)self);
++tstate->trash_delete_nesting;So this seems to be a known problem which deserves to be fixed properly. jdemeyer commented on Feb 14, 2019 ContributorAuthor More actions
Jeroen, could you share your example? I am learning the C-API of Python and this example could be interesting.
Either use the Cython code I posted above or run the testcase I added in PR 11841. serhiy-storchaka commented on Mar 27, 2019 Disabling the trashcan mechanism returns the problem for solving which the trashcan mechanism was introduced.
>>> from _testcapi import MyList
>>> L = None
>>> for i in range(1000000):
... L = MyList((L,))
...
>>> del L
Segmentation fault (core dumped) I think we need better way to resolve this issue.
For example, setting the object type to the parent type should solve this issue. jdemeyer commented on Mar 27, 2019 ContributorAuthor More actions Yes of course. When not using the trashcan, stuff crashes. I don't get your point... jdemeyer commented on Mar 27, 2019 ContributorAuthor More actions To clarify: the purpose of MyList is specifically to check that no double deallocations occur. For this test to make sense, MyList should not use the trashcan itself. serhiy-storchaka commented on Mar 28, 2019 In your example you can add
PyTypeObject *tp = Py_TYPE(self);
Py_TYPE(self) = &PyList_Type;
if (PyType_GetFlags(tp) & Py_TPFLAGS_HEAPTYPE) {
Py_DECREF(tp);
}before calling PyList_Type.tp_dealloc().
It is possible to add such code directly in PyList_Type.tp_dealloc and other deallocators that use the trashcan mechanism. jdemeyer commented on Mar 28, 2019 ContributorAuthor More actions Changing types like that looks like an ugly hack and a recipe for breakage. For example, in list_dealloc(), the following needs the type to be correct:
if (numfree < PyList_MAXFREELIST && PyList_CheckExact(op))
free_list[numfree++] = op;
else
Py_TYPE(op)->tp_free((PyObject *)op);Could you please clarify your opinion: do you think that there's something wrong with PR 11841? And if yes: what's wrong with it? Or are you just giving optional suggestions? serhiy-storchaka commented on Mar 28, 2019 Yes, I think that there's something wrong with PR 11841. It disables the trashcan mechanism and does not provide other mechanism for solving that problem. jdemeyer commented on Mar 28, 2019 ContributorAuthor More actions
It disables the trashcan mechanism
Yes, it disables the trashcan in some cases. But only when using the trashcan mechanism would probably crash CPython anyway due to a double deallocation. So at the very least, PR 11841 improves things from "crashing whenever the trashcan is used" to "crashing on stack overflows".
Do you have a real example where PR 11841 actually makes things *worse*?
and does not provide other mechanism for solving that problem.
The recommended thing to do is that the subclass also implements the trashcan. See OrderedDict for an example: both the base class "dict" and the subclass "OrderedDict" use the trashcan. jdemeyer commented on Mar 29, 2019 ContributorAuthor More actions I created an additional PR 12607 with some more changes to the, in particular to make the old backwards-compatibility trashcan macros safer. This should be seen as part of the same bugfix but I decided to make a new PR because PR 11841 had several positive reviews already. jdemeyer commented on Apr 5, 2019 ContributorAuthor More actions I realized that there is a nasty interaction between the trashcan and __del__: if you're very close to the trashcan limit and you're calling __del__, then objects that should have been deallocated in __del__ (in particular, an object involving self) might instead end up in the trashcan. So self might be resurrected when it shouldn't be.
This is causing test failures in the 2.7 backport due to a different implementation of __del__. jdemeyer commented on Apr 8, 2019 ContributorAuthor More actions In Python 3, the resurrection issue probably appears too. But it's not so much a problem since __del__ (mapped to tp_finalize) is only called once anyway. So there are no bad consequences if the object is resurrected incorrectly. pitrou commented on May 10, 2019 kumaraditya303 commented on Jun 19, 2022 Closing as it seems fixed, feel free to reopen if required.
MetadataMetadata3.8 (EOL)end of lifeend of lifeinterpreter-core(Objects, Python, Grammar, and Parser dirs)(Objects, Python, Grammar, and Parser dirs)type-bugAn unexpected behavior, bug, or errorAn unexpected behavior, bug, or error DevelopmentNo branches or pull requestsIssue actions
|