classification
Title: tp_dealloc trashcan shouldn't be called for subclasses
Type: behavior Stage: patch review
Components: Interpreter Core Versions: Python 3.8, Python 3.7, Python 2.7
process
Status: open Resolution:
Dependencies: Superseder:
Assigned To: Nosy List: eric.snow, jdemeyer, matrixise, pitrou, scoder
Priority: normal Keywords: patch

Created on 2019-02-13 11:02 by jdemeyer, last changed 2019-02-14 09:06 by jdemeyer.

Pull Requests
URL Status Linked Edit
PR 11841 open jdemeyer, 2019-02-13 15:52
Messages (8)
msg335405 - (view) Author: Jeroen Demeyer (jdemeyer) * Date: 2019-02-13 11:02
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
msg335406 - (view) Author: Stéphane Wirtel (matrixise) * (Python triager) Date: 2019-02-13 11:08
Jeroen, could you share your example? I am learning the C-API of Python and this example could be interesting.
msg335440 - (view) Author: Jeroen Demeyer (jdemeyer) * Date: 2019-02-13 14:00
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
msg335453 - (view) Author: Eric Snow (eric.snow) * (Python committer) Date: 2019-02-13 15:34
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.
msg335455 - (view) Author: Antoine Pitrou (pitrou) * (Python committer) Date: 2019-02-13 15:42
By "relatively new thing", you mean less than 20 years old? :-)
msg335463 - (view) Author: Eric Snow (eric.snow) * (Python committer) Date: 2019-02-13 16:28
On Wed, Feb 13, 2019 at 8:42 AM Antoine Pitrou <report@bugs.python.org> wrote:
> Antoine Pitrou <pitrou@free.fr> 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. :)
msg335476 - (view) Author: Jeroen Demeyer (jdemeyer) * Date: 2019-02-13 20:51
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.
msg335513 - (view) Author: Jeroen Demeyer (jdemeyer) * Date: 2019-02-14 09:06
> 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.
History
Date User Action Args
2019-02-14 09:06:42jdemeyersetmessages: + msg335513
2019-02-13 20:51:51jdemeyersetmessages: + msg335476
2019-02-13 16:28:05eric.snowsetmessages: + msg335463
2019-02-13 15:52:37jdemeyersetkeywords: + patch
stage: test needed -> patch review
pull_requests: + pull_request11871
2019-02-13 15:42:39pitrousetmessages: + msg335455
2019-02-13 15:34:50eric.snowsetnosy: + eric.snow
messages: + msg335453

type: behavior
stage: test needed
2019-02-13 14:00:11jdemeyersetmessages: + msg335440
2019-02-13 11:08:00matrixisesetnosy: + matrixise
messages: + msg335406
2019-02-13 11:02:31jdemeyercreate