Fix MethodBinding/OverloadMapper memory leak (#691)#2719
Conversation
MethodBinding and OverloadMapper held PyObject `target` references that were not disposed during tp_clear, leaving Python-side refcount drops to wait on the multi-hop .NET finalizer chain. They also shared the same C# PyObject instance across mp_subscript/Overloads paths, so freeing one could free the underlying Python object out from under the others. - ExtensionType: add virtual OnClear() hook called from tp_clear before the GCHandle is released, letting subclasses eagerly drop owned Python references. - MethodBinding/OverloadMapper: override OnClear to dispose `target`. (`targetType` is intentionally not disposed since Python types are long-lived and tracked by other caches.) - Take an independent INCREF'd PyObject copy at every site that hands a shared target into a new MethodBinding or OverloadMapper, so each wrapper owns its own reference. Result: the three _does_not_leak_memory tests drop from ~485 MB delta to ~10 KB delta on Python 3.14.
The previous 90% threshold (0.9 MB/iter against a 1 MB allocation) documented the issue but did not reproduce it: master leaks ~600-765 KB/iter, which the 0.9 MB threshold accepts as passing. Drop the threshold to 10% (104 KB/iter). On the 2026-05-09 verification run with Python 3.14 GIL on linux-aarch64: Without fix (master): ~572-765 KB/iter (FAIL) With fix (this branch): ~-500 B/iter (PASS) Margin is roughly 6x in either direction across .NET 8 and .NET 10, so the threshold cleanly separates buggy from fixed states without being sensitive to GC noise.
|
If the objects are not shared anymore and are always owned by |
| /// <summary> | ||
| /// Called during tp_clear before the GCHandle is released. | ||
| /// Override to eagerly dispose Python object references (PyObject fields) | ||
| /// held by the subclass, preventing the multi-hop .NET finalizer chain | ||
| /// from delaying Python-side refcount decrements. | ||
| /// </summary> | ||
| protected virtual void OnClear() { } | ||
|
|
There was a problem hiding this comment.
Is there a reason to not have ExtensionType be IDisposable instead of exposing OnClear?
There was a problem hiding this comment.
I think tp_clear and Dispose have different contracts. tp_clear releases references to break GC cycles but the Python object can still be reachable afterward, whereas Dispose should mean that the instance is dead. ExtensionType instances are owned by Python's GC, not by .NET callers — there's no using site and no one calls Dispose() on them. OnClear makes it clear that it's a hook fired from tp_clear.
|
Ownership is still explicit: |
Fixes #691.
Cause
MethodBindingandOverloadMapperhold aPyObject targetbut didn't release it ontp_clear, so the underlying CLR instance waited on the .NET finalizer chain to drop the refcount. They also shared the same C#PyObjectinstance acrossmp_subscript/Overloadspaths, so disposing one wrapper corrupted the others.Fix
ExtensionType: add virtualOnClear()hook called fromtp_clear.MethodBinding/OverloadMapper: overrideOnClearto disposetarget. (targetTypeleft alone — disposing it broke unrelated subclass tests.)new PyObject(self.target.Reference)so each wrapper owns its own INCREF'd reference.Tests
The three existing
*_does_not_leak_memorytests cover the three sharing sites but their 0.9 MB/iter threshold was too loose — master was leaking ~600 KB/iter and still passing. Tightened to 0.1 MB/iter (104 KB).Verification (Python 3.14 GIL, linux-aarch64)