bpo-44897: WIP: Integrate trashcan into _Py_Dealloc #27738
Closed
+139
−336
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This is a WIP/proof-of-concept of doing away with Py_TRASHCAN_BEGIN and Py_TRASHCAN_END and instead integrating the functionality into _Py_Dealloc. There are a few advantages: - all container objects have the risk of overflowing the C stack if a long reference chain of them is created and then deallocated. So, to be safe, the tp_dealloc methods for those objects should be protected from overflowing the stack. - the Py_TRASHCAN_BEGIN and Py_TRASHCAN_END macros are hard to understand and a bit hard to use correctly. Making the mechanism internal avoids confusion. The code can be slightly simplified as well. This proof-of-concept seems to pass tests but it will need some careful review. The exact rules related to calling GC Track/Untrack are subtle and this changes things a bit. I.e. tp_dealloc is called with GC objects already untracked. For 3rd party extensions, they are calling PyObject_GC_UnTrack() and so I believe they should still work. The fact that PyObject_CallFinalizerFromDealloc() wants GC objects to definitely be tracked is a bit of a mystery to me (there is an assert to check that). I changed the code to track objects if they are not already tracked but I'm not sure that's correct. There could be a performance hit, due to the _PyType_IS_GC() test that was added to the _Py_Dealloc() function. For non-GC objects, that's going to be a new branch and I'm worried it might hurt a bit. OTOH, maybe it's just in the noise. Profiling will need to be done.
nascheme
requested review from
1st1,
markshannon,
methane and
rhettinger
as code owners
Since _Py_Dealloc will untrack, don't need to do it in tp_dealloc. If an object is resurrected, PyObject_CallFinalizerFromDealloc() will do the track call.
I'm not sure _PyType_IS_GC() is safe in case the object returns false from tp_is_gc().
|
This PR is stale because it has been open for 30 days with no activity. |
github-actions
bot
removed
the
stale
|
FYI, I believe this would have fixed GH-102356 and probably a lot of similar crashing bugs that have not yet been found. I would imagine most "container" (GC tracked) objects can be used to blow up the C stack on dealloc. |
|
Since this PR is so out of date, I've created a new one: GH-124716 |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
This is a WIP/proof-of-concept of doing away with Py_TRASHCAN_BEGIN and
Py_TRASHCAN_END and instead integrating the functionality into
_Py_Dealloc. There are a few advantages:
all container objects have the risk of overflowing the C stack if a
long reference chain of them is created and then deallocated. So, to
be safe, the tp_dealloc methods for those objects should be protected
from overflowing the stack.
the Py_TRASHCAN_BEGIN and Py_TRASHCAN_END macros are hard to
understand and a bit hard to use correctly. Making the mechanism
internal avoids confusion. The code can be slightly simplified as
well.
This proof-of-concept seems to pass tests but it will need some careful
review. The exact rules related to calling GC Track/Untrack are subtle
and this changes things a bit. I.e. tp_dealloc is called with GC
objects already untracked. For 3rd party extensions, they are calling
PyObject_GC_UnTrack() and so I believe they should still work.
The fact that PyObject_CallFinalizerFromDealloc() wants GC objects to
definitely be tracked is a bit of a mystery to me (there is an assert to
check that). I changed the code to track objects if they are not
already tracked but I'm not sure that's correct.
There could be a performance hit, due to the _PyType_IS_GC() test that
was added to the _Py_Dealloc() function. For non-GC objects, that's
going to be a new branch and I'm worried it might hurt a bit. OTOH,
maybe it's just in the noise. Profiling will need to be done.
https://bugs.python.org/issue44897