Uh oh!
There was an error while loading.Please reload this page.
- Notifications
You must be signed in to change notification settings - Fork32k
gh-119127: Fix _functools.Placeholder singleton#124601
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 ourterms of service andprivacy statement. We’ll occasionally send you account related emails.
Already on GitHub?Sign in to your account
Uh oh!
There was an error while loading.Please reload this page.
Conversation
* The module state now stores a strong reference to the Placeholder singleton.* Use a regular dealloc function.* Add Py_TPFLAGS_HAVE_GC flag and traverse function to help the GC to collect the type when _functools extension is unloaded.
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.
This looks like the right fix. I haven't manually confirmed that this fixes the refleak, but this is the proper way to implement singletons. (Originally, it was just an immortal object that never gets freed, and therefore causes a leak -- it gets worse as new subinterpreters are created and the module is reset.)
Thank you, Victor! Should this be targetinggh-124586 rather thangh-119127?
I checked manually that test_ast no longer leaks with this change. |
!buildbot refleaks |
bedevere-bot commentedSep 26, 2024
🤖 New build scheduled with the buildbot fleet by@Eclips4 for commit37e994a 🤖 The command will test the builders whose names match following regular expression: The builders matched are:
|
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. I've checked it locally and there's no leak intest_ast with this patch applied.
I run buildbots to make sure that everything is fine.
| _Py_SetImmortal(placeholder); | ||
| PyObject_GC_UnTrack(self); | ||
| PyTypeObject *tp = Py_TYPE(self); | ||
| tp->tp_free((PyObject*)self); |
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.
| tp->tp_free((PyObject*)self); | |
| tp->tp_free(self); |
nit: there's no need to casting it toPyObject * since it's already aPyObject *
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.
I will wait for a few Refleaks buildbots to complete before merging this change. |
Four Refleaks workers completed: test_datetime still leaks, but that's unrelated to this fix. test_ast no longer leaks on these workers. I consider that the fix works as expected and merge my PR. |
257a20a intopython:mainUh oh!
There was an error while loading.Please reload this page.
See#124606 for test_datetime leak. |
Uh oh!
There was an error while loading.Please reload this page.