gh-119127: Fix _functools.Placeholder singleton #124601
Conversation
vstinner
commented
Sep 26, 2024
•
vstinner
commented
- 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.
- Issue: functools.partial placeholders #119127
* 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.
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 targeting gh-124586 rather than gh-119127?
I checked manually that test_ast no longer leaks with this change. |
|
!buildbot refleaks |
|
🤖 New build scheduled with the buildbot fleet by @Eclips4 for commit 37e994a 🤖 The command will test the builders whose names match following regular expression: The builders matched are:
|
| _Py_SetImmortal(placeholder); | ||
| PyObject_GC_UnTrack(self); | ||
| PyTypeObject *tp = Py_TYPE(self); | ||
| tp->tp_free((PyObject*)self); |
There was a problem hiding this comment.
| tp->tp_free((PyObject*)self); | |
| tp->tp_free(self); |
nit: there's no need to casting it to PyObject * since it's already a PyObject *
|
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. |
See #124606 for test_datetime leak. |
