-
-
Notifications
You must be signed in to change notification settings - Fork 31k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
gh-94673: Always Finalize Static Builtin Types #95153
gh-94673: Always Finalize Static Builtin Types #95153
Conversation
@vstinner, you introduced the original skipping logic that I'm basically removing. Are there any problems with the approach I'm taking? |
This was split out of #94995. |
🤖 New build scheduled with the buildbot fleet by @ericsnowcurrently for commit 74d5af3 🤖 If you want to schedule another build, you need to add the ":hammer: test-with-buildbots" label again. |
Hmm, this would affected extension modules which store |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
Co-authored-by: Kumar Aditya <[email protected]>
I skipped type deallocation if they have sub-types to prevent crashes. I don't recall the details. I got issues with subclasses: https://bugs.python.org/issue46417#msg411122 I fixed this issue with: 8ee07dd It's a complicated issue: https://bugs.python.org/issue46417#msg411304 See also #84258 |
If Python is initialized multiple times (Py_Initialize + Py_Finalize called multiple times), a sub-type may try to get data from its parent static type which has been cleared. If the static type is not properly initialized, Python can crash. If the heap type is used during the finalization after the static type has been cleared, Python can crash. Again, I don't recall the details. I don't know the crashes that I got. |
Static builtin types are finalized by calling
_PyStaticType_Dealloc()
. Currently we skip finalizing such a type if it still has subtypes (i.e. its tp_subclasses hasn't been cleared yet). The problem is that static builtin types hold several heap objects, which leak if we skip the type's finalization. This PR addresses that.For context, there's an old comment that says the following:
However, it isn't clear that is actually still true. Clearing tp_dict should mean it isn't a problem.
Furthermore, the only subtypes that might still be around come from extension modules that didn't clean them up when unloaded (i.e. extensions that do not implement multi-phase initialization, AKA PEP 489). Those objects are already leaking, so this PR doesn't change anything in that regard. Instead, this change means more objects gets cleaned up that before.