Consider integration of PyObject_GC_UnTrack() with the trashcan C API

[nascheme]

BPO	44881
Nosy	@tim-one, @nascheme, @vstinner, @pablogsal
PRs	bpo-44881: Integrate GC untrack into trashcan begin. #27718

^{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 2021-08-10.21:07:59.473>
labels = ['type-feature']
title = 'Consider integration of PyObject_GC_UnTrack() with the trashcan C API'
updated_at = <Date 2021-08-16.21:40:17.744>
user = 'https://github.com/nascheme'

bugs.python.org fields:

activity = <Date 2021-08-16.21:40:17.744>
actor = 'nascheme'
assignee = 'none'
closed = False
closed_date = None
closer = None
components = []
creation = <Date 2021-08-10.21:07:59.473>
creator = 'nascheme'
dependencies = []
files = []
hgrepos = []
issue_num = 44881
keywords = ['patch']
message_count = 15.0
messages = ['399356', '399358', '399359', '399360', '399367', '399377', '399378', '399379', '399426', '399434', '399437', '399495', '399519', '399548', '399686']
nosy_count = 4.0
nosy_names = ['tim.peters', 'nascheme', 'vstinner', 'pablogsal']
pr_nums = ['27718']
priority = 'normal'
resolution = None
stage = 'needs patch'
status = 'open'
superseder = None
type = 'enhancement'
url = 'https://bugs.python.org/issue44881'
versions = []

[nascheme]

The fix for bpo-33930 includes a somewhat mysterious comment:

// The Py_TRASHCAN mechanism requires that we be able to
// call PyObject_GC_UnTrack twice on an object.

I wonder if we can just integrate the untrack into the body of the trashcan code. Then, the explicit call to untrack in the dealloc function body can be removed. That removes the risk of incorrectly using the macro version.

I suspect the reason this was not done originally is because the original trashcan mechanism did not use the GC header pointers to store objects. Now, any object that uses the trashcan *must* be a GC object.

[pablogsal]

We probably will need a new set of macros because these macros are public IIRC correctly. Although as PyObject_GC_UnTrack has a check, we probably can just absorb it and let backwards compat work by being idempotent if called twice

[nascheme]

Extensions that call PyObject_GC_UnTrack before calling Py_TRASHCAN_BEGIN will still work, they will just take a very minor performance hit. I don't think it is worth the trouble to introduce new macros for that reason. Extensions that really care about performance can wrap the call in a Python version ifdef.

There is an issue if someone writes and tests their extension with the new API, i.e. without having the explicit PyObject_GC_UnTrack() call in their dealloc method. If they compile with an older Python, they likely get a crash. If they compile with asserts enable, they would get an assert fail in _PyTrash_thread_deposit_object, i.e.:

_PyObject_ASSERT(op, !_PyObject_GC_IS_TRACKED(op));

I guess that's an argument for new macros.

[nascheme]

For examples of bugs caused by forgetting the untrack calls, see bpo-31095.

[nascheme]

Since C99 is now allowed, perhaps we should suggest that declarations go after Py_TRASHCAN_BEGIN, e.g.

	mytype_dealloc(mytype *p)
	{
	 Py_TRASHCAN_BEGIN(p, mytype_dealloc)
	 ... declarations go here ...
	 ... The body of the deallocator goes here, including all calls ...
	 ... to Py_DECREF on contained objects. ...
	 Py_TRASHCAN_END // there should be no code after this
	}

The only dealloc method that doesn't fit this pattern is subtype_dealloc() and that's because the object might not be a GC object.

Given the pattern, it occurs to me that that _Py_Dealloc() could do the trashcan begin/end work. However, the authors of the dealloc methods would have to realize the special rules of the trashcan (e.g. no returns from the dealloc method body). Also, there would be some overhead added to _Py_Dealloc() to check if the trashcan is supported. E.g. checking a type flag.

Another idea would be to add a new slot for the method, e.g. tp_dealloc_trash. Then, _Py_Dealloc() could check if it exists and if so do the trashcan begin/end dance around it. That would still add some overhead to _Py_Dealloc() so I think the current macros are the best approach.

[vstinner]

Please don't add new macros. The TRASHCAN C API is already complex enough.

[vstinner]

Moreover, before making any change, I would suggest by starting with documenting the existing trashcan C API. Right now, there is no documentation in Doc/c-api/. Documentation can be copied from Include/cpython/object.h.

Last year, I modified the trashcan API to hide implementation details. There were also issues with the stable ABI... thing which is unclear to me, since the trashcan C API is excluded from the limited C API. Well, I excluded it. Previously, it was part of it, but it did not work with the limited C API.

commit 0fa4f43
Author: Victor Stinner <vstinner@python.org>
Date: Wed Feb 5 12:23:27 2020 +0100

bpo-39542: Exclude trashcan from the limited C API (GH-18362)

Exclude trashcan mechanism from the limited C API: it requires access to
PyTypeObject and PyThreadState structure fields, whereas these structures
are opaque in the limited C API.

The trashcan mechanism never worked with the limited C API. Move it
from object.h to cpython/object.h.

commit 38965ec
Author: Victor Stinner <vstinner@python.org>
Date: Fri Mar 13 16:51:52 2020 +0100

bpo-39947: Hide implementation detail of trashcan macros (GH-18971)

Py_TRASHCAN_BEGIN_CONDITION and Py_TRASHCAN_END macro no longer
access PyThreadState attributes, but call new private
_PyTrash_begin() and _PyTrash_end() functions which hide
implementation details.

commit db532a0
Author: Victor Stinner <vstinner@python.org>
Date: Wed Jun 23 15:51:47 2021 +0200

bpo-39947: Remove old private trashcan C API functions (GH-26869)

Remove 4 C API private trashcan functions which were only kept for
the backward compatibility of the stable ABI with Python 3.8 and
older, since the trashcan API was not usable with the limited C API
on Python 3.8 and older. The trashcan API was excluded from the
limited C API in Python 3.9.

Removed functions:

* _PyTrash_deposit_object()
* _PyTrash_destroy_chain()
* _PyTrash_thread_deposit_object()
* _PyTrash_thread_destroy_chain()

The trashcan C API was never usable with the limited C API, since old
trashcan macros accessed directly PyThreadState members like
"_tstate->trash_delete_nesting", whereas the PyThreadState structure
is opaque in the limited C API.

Exclude also the PyTrash_UNWIND_LEVEL constant from the C API.

The trashcan C API was modified in Python 3.9 by commit
38965ec5411da60d312b59be281f3510d58e0cf1 and in Python 3.10 by commit
ed1a5a5baca8f61e9a99c5be3adc16b1801514fe to hide implementation
details.

[vstinner]

Now, any object that uses the trashcan *must* be a GC object.

It would be nice to add an assertion in _PyTrash_begin().

[nascheme]

I was thinking about this more today and I think the better fix is to actually build the trashcan mechanism into _Py_Dealloc(). Requiring that types opt-in to the trashcan mechanism by using the trashcan macros is not ideal.

First, the trashcan macros are a bit tricky to use correctly. Second, every "container" type is potentially a part of a long ref chain and could blow up the stack on deallocation (i.e. triggered from DECREF). So, for correctness/robustness, every type that supports cyclic GC should get trashcan-style deallocation.

We would have to find a way to do this without incurring a (significant) performance hit. Also, it would have to be done without breaking C extensions. Ideally they would get trashcan-style deallocation without any source code changes. I'm not sure if that can be done but I'm hopeful it's possible.

[nascheme]

I wrote a proof-of-concept as bpo-44897. PR 27718 (this issue) might a slightly better performance but I like the other one better because it doesn't expose so much implementation detail to extension types. Either of them require careful review before merging.

[vstinner]

The problem of PyObject_GC_UnTrack() is just the most visible effect of the trashcan mecanism: tp_dealloc can be called twice, and this is *not* expected by the tp_dealloc API.

Putting trashcan mecanism outside tp_dealloc can allow to make sure that tp_dealloc is called exactly once. _Py_Dealloc() sounds like a good candidate, but I didn't check if it's the only way to call tp_dealloc. Are there other places where tp_dealloc is called *directly*?

Using military grade regex, I found the following functions calling tp_dealloc:

grep 'dealloc[) ]([a-zA-Z]\+)' */.c

• _Py_Dealloc() obviously
• _PyTrash_thread_destroy_chain()
• subtype_dealloc()
• Modules/_testcapimodule.c: check_pyobject_freed_is_freed() <= unit test, it can be ignored

So if we move the trashcan usage inside functions, 3 functions must be modified:

• _Py_Dealloc()
• _PyTrash_thread_destroy_chain()
• subtype_dealloc()