From a305127d13b546c8a2ab73b058daed42a72fa873 Mon Sep 17 00:00:00 2001 From: greateggsgreg Date: Sat, 9 May 2026 16:17:05 -0400 Subject: [PATCH 1/3] Fix MethodBinding/OverloadMapper memory leak (#691) 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. --- src/runtime/Types/ExtensionType.cs | 10 ++++++++++ src/runtime/Types/MethodBinding.cs | 14 ++++++++++++-- src/runtime/Types/OverloadMapper.cs | 13 +++++++++++-- 3 files changed, 33 insertions(+), 4 deletions(-) diff --git a/src/runtime/Types/ExtensionType.cs b/src/runtime/Types/ExtensionType.cs index 305fdc15d..114f2d706 100644 --- a/src/runtime/Types/ExtensionType.cs +++ b/src/runtime/Types/ExtensionType.cs @@ -84,8 +84,18 @@ public unsafe static void tp_dealloc(NewReference lastRef) DecrefTypeAndFree(lastRef.Steal()); } + /// + /// 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. + /// + protected virtual void OnClear() { } + public static int tp_clear(BorrowedReference ob) { + (GetManagedObject(ob) as ExtensionType)?.OnClear(); + var weakrefs = Runtime.PyObject_GetWeakRefList(ob); if (weakrefs != null) { diff --git a/src/runtime/Types/MethodBinding.cs b/src/runtime/Types/MethodBinding.cs index bfe22b0f3..dfcf570a0 100644 --- a/src/runtime/Types/MethodBinding.cs +++ b/src/runtime/Types/MethodBinding.cs @@ -54,7 +54,10 @@ public static NewReference mp_subscript(BorrowedReference tp, BorrowedReference } MethodObject overloaded = self.m.WithOverloads(overloads); - var mb = new MethodBinding(overloaded, self.target, self.targetType); + var mb = new MethodBinding( + overloaded, + self.target is null ? null : new PyObject(self.target.Reference), + self.targetType is null ? null : new PyType(self.targetType.Reference)); return mb.Alloc(); } @@ -141,7 +144,8 @@ public static NewReference tp_getattro(BorrowedReference ob, BorrowedReference k // FIXME: deprecate __overloads__ soon... case "__overloads__": case "Overloads": - var om = new OverloadMapper(self.m, self.target); + var om = new OverloadMapper(self.m, + self.target is null ? null : new PyObject(self.target.Reference)); return om.Alloc(); case "__signature__" when Runtime.InspectModule is not null: var sig = self.Signature; @@ -281,5 +285,11 @@ public static NewReference tp_repr(BorrowedReference ob) string name = self.m.name; return Runtime.PyString_FromString($"<{type} method '{name}'>"); } + + protected override void OnClear() + { + target?.Dispose(); + target = null; + } } } diff --git a/src/runtime/Types/OverloadMapper.cs b/src/runtime/Types/OverloadMapper.cs index 8f6e30478..6d02e2fee 100644 --- a/src/runtime/Types/OverloadMapper.cs +++ b/src/runtime/Types/OverloadMapper.cs @@ -10,7 +10,7 @@ namespace Python.Runtime internal class OverloadMapper : ExtensionType { private readonly MethodObject m; - private readonly PyObject? target; + private PyObject? target; public OverloadMapper(MethodObject m, PyObject? target) { @@ -42,7 +42,10 @@ public static NewReference mp_subscript(BorrowedReference tp, BorrowedReference return Exceptions.RaiseTypeError(e); } - var mb = new MethodBinding(self.m, self.target) { info = mi }; + var mb = new MethodBinding( + self.m, + self.target is null ? null : new PyObject(self.target.Reference)) + { info = mi }; return mb.Alloc(); } @@ -54,5 +57,11 @@ public static NewReference tp_repr(BorrowedReference op) var self = (OverloadMapper)GetManagedObject(op)!; return self.m.GetDocString(); } + + protected override void OnClear() + { + target?.Dispose(); + target = null; + } } } From 7f8a2475f970313251eefba38f597f16b8077660 Mon Sep 17 00:00:00 2001 From: greateggsgreg Date: Sat, 9 May 2026 16:44:08 -0400 Subject: [PATCH 2/3] Tighten leak-test threshold to 10% to actually fail the bug 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. --- tests/test_method.py | 15 ++++++++------- 1 file changed, 8 insertions(+), 7 deletions(-) diff --git a/tests/test_method.py b/tests/test_method.py index 7820457d5..53c614498 100644 --- a/tests/test_method.py +++ b/tests/test_method.py @@ -983,9 +983,10 @@ def test_getting_generic_method_binding_does_not_leak_memory(memory_usage_tracki bytesAllocatedPerIteration = pow(2, 20) # 1MB bytesLeakedPerIteration = processBytesDelta / iterations - # Allow 90% threshold - this shows the original issue is fixed, which leaks the full allocated bytes per iteration - # Increased from 50% to ensure that it works on Windows with Python >3.13 - failThresholdBytesLeakedPerIteration = bytesAllocatedPerIteration * 0.9 + # Tight 10% threshold: with the fix the per-iteration leak is essentially + # zero, while the bug retains the bulk of the 1 MB payload (~600 KB/iter + # on 3.14 GIL). 100 KB/iter cleanly distinguishes the two states. + failThresholdBytesLeakedPerIteration = bytesAllocatedPerIteration * 0.1 assert bytesLeakedPerIteration < failThresholdBytesLeakedPerIteration @@ -1025,8 +1026,8 @@ def test_getting_overloaded_method_binding_does_not_leak_memory(memory_usage_tra bytesAllocatedPerIteration = pow(2, 20) # 1MB bytesLeakedPerIteration = processBytesDelta / iterations - # Allow 90% threshold - this shows the original issue is fixed, which leaks the full allocated bytes per iteration - failThresholdBytesLeakedPerIteration = bytesAllocatedPerIteration * 0.9 + # Tight 10% threshold; see test_getting_generic_method_binding_does_not_leak_memory. + failThresholdBytesLeakedPerIteration = bytesAllocatedPerIteration * 0.1 assert bytesLeakedPerIteration < failThresholdBytesLeakedPerIteration @@ -1068,8 +1069,8 @@ def test_getting_method_overloads_binding_does_not_leak_memory(memory_usage_trac bytesAllocatedPerIteration = pow(2, 20) # 1MB bytesLeakedPerIteration = processBytesDelta / iterations - # Allow 90% threshold - this shows the original issue is fixed, which leaks the full allocated bytes per iteration - failThresholdBytesLeakedPerIteration = bytesAllocatedPerIteration * 0.9 + # Tight 10% threshold; see test_getting_generic_method_binding_does_not_leak_memory. + failThresholdBytesLeakedPerIteration = bytesAllocatedPerIteration * 0.1 assert bytesLeakedPerIteration < failThresholdBytesLeakedPerIteration From 4c18a166e3f4c9d6f12cfb2a86ea1a01da7572f0 Mon Sep 17 00:00:00 2001 From: Benedikt Reinartz Date: Mon, 11 May 2026 17:23:19 +0200 Subject: [PATCH 3/3] Bugfix and improvements - Handle the `PyType` reference in `OverloadMapper` and `MethodBinding` in the same way as the object reference - Unconditionally store the `PyType` of the object - Introduce `NewReference` helper function for the object and type passing - Fix the remaining missing reference count bump for the type (`MethodObject`) - As the count is now correct, `Dispose` the type as well --- src/runtime/PythonTypes/PyObject.cs | 6 ++++++ src/runtime/PythonTypes/PyType.cs | 6 ++++++ src/runtime/Types/MethodBinding.cs | 18 ++++++------------ src/runtime/Types/MethodObject.cs | 2 +- src/runtime/Types/OverloadMapper.cs | 10 +++++----- 5 files changed, 24 insertions(+), 18 deletions(-) diff --git a/src/runtime/PythonTypes/PyObject.cs b/src/runtime/PythonTypes/PyObject.cs index cf0c2a03f..1949710fb 100644 --- a/src/runtime/PythonTypes/PyObject.cs +++ b/src/runtime/PythonTypes/PyObject.cs @@ -93,6 +93,12 @@ internal PyObject(in StolenReference reference) Finalizer.Instance.ThrottledCollect(); } + /// + /// Create a new PyObject instance of this object, bumping the reference + /// count. + /// + public PyObject NewReference() => new(this); + // Ensure that encapsulated Python object is decref'ed appropriately // when the managed wrapper is garbage-collected. ~PyObject() diff --git a/src/runtime/PythonTypes/PyType.cs b/src/runtime/PythonTypes/PyType.cs index 28bda5d3e..dd82450db 100644 --- a/src/runtime/PythonTypes/PyType.cs +++ b/src/runtime/PythonTypes/PyType.cs @@ -35,6 +35,12 @@ internal PyType(in StolenReference reference, bool prevalidated = false) : base( throw new ArgumentException("object is not a type"); } + /// + /// Create a new PyType instance of this object, bumping the reference + /// count. + /// + public new PyType NewReference() => new(this); + protected PyType(SerializationInfo info, StreamingContext context) : base(info, context) { } internal new static PyType? FromNullableReference(BorrowedReference reference) diff --git a/src/runtime/Types/MethodBinding.cs b/src/runtime/Types/MethodBinding.cs index dfcf570a0..b68f338ff 100644 --- a/src/runtime/Types/MethodBinding.cs +++ b/src/runtime/Types/MethodBinding.cs @@ -18,14 +18,12 @@ internal class MethodBinding : ExtensionType internal MaybeMethodInfo info; internal MethodObject m; internal PyObject? target; - internal PyType? targetType; + internal PyType targetType; - public MethodBinding(MethodObject m, PyObject? target, PyType? targetType = null) + public MethodBinding(MethodObject m, PyObject? target, PyType targetType) { this.target = target; - - this.targetType = targetType ?? target?.GetPythonType(); - + this.targetType = targetType; this.info = null; this.m = m; } @@ -54,10 +52,7 @@ public static NewReference mp_subscript(BorrowedReference tp, BorrowedReference } MethodObject overloaded = self.m.WithOverloads(overloads); - var mb = new MethodBinding( - overloaded, - self.target is null ? null : new PyObject(self.target.Reference), - self.targetType is null ? null : new PyType(self.targetType.Reference)); + var mb = new MethodBinding(overloaded, self.target?.NewReference(), self.targetType.NewReference()); return mb.Alloc(); } @@ -144,8 +139,7 @@ public static NewReference tp_getattro(BorrowedReference ob, BorrowedReference k // FIXME: deprecate __overloads__ soon... case "__overloads__": case "Overloads": - var om = new OverloadMapper(self.m, - self.target is null ? null : new PyObject(self.target.Reference)); + var om = new OverloadMapper(self.m, self.target?.NewReference(), self.targetType.NewReference()); return om.Alloc(); case "__signature__" when Runtime.InspectModule is not null: var sig = self.Signature; @@ -253,7 +247,6 @@ public static NewReference tp_call(BorrowedReference ob, BorrowedReference args, } } - /// /// MethodBinding __hash__ implementation. /// @@ -289,6 +282,7 @@ public static NewReference tp_repr(BorrowedReference ob) protected override void OnClear() { target?.Dispose(); + targetType.Dispose(); target = null; } } diff --git a/src/runtime/Types/MethodObject.cs b/src/runtime/Types/MethodObject.cs index 12484d301..1bb3083c0 100644 --- a/src/runtime/Types/MethodObject.cs +++ b/src/runtime/Types/MethodObject.cs @@ -197,7 +197,7 @@ public static NewReference tp_descr_get(BorrowedReference ds, BorrowedReference && self.type.Value.IsInstanceOfType(obj.inst)) { var basecls = ReflectedClrType.GetOrCreate(self.type.Value); - return new MethodBinding(self, new PyObject(ob), basecls).Alloc(); + return new MethodBinding(self, new PyObject(ob), basecls.NewReference()).Alloc(); } return new MethodBinding(self, target: new PyObject(ob), targetType: new PyType(tp)).Alloc(); diff --git a/src/runtime/Types/OverloadMapper.cs b/src/runtime/Types/OverloadMapper.cs index 6d02e2fee..79130a669 100644 --- a/src/runtime/Types/OverloadMapper.cs +++ b/src/runtime/Types/OverloadMapper.cs @@ -11,10 +11,12 @@ internal class OverloadMapper : ExtensionType { private readonly MethodObject m; private PyObject? target; + readonly PyType targetType; - public OverloadMapper(MethodObject m, PyObject? target) + public OverloadMapper(MethodObject m, PyObject? target, PyType targetType) { this.target = target; + this.targetType = targetType; this.m = m; } @@ -42,10 +44,7 @@ public static NewReference mp_subscript(BorrowedReference tp, BorrowedReference return Exceptions.RaiseTypeError(e); } - var mb = new MethodBinding( - self.m, - self.target is null ? null : new PyObject(self.target.Reference)) - { info = mi }; + var mb = new MethodBinding(self.m, self.target?.NewReference(), self.targetType.NewReference()) { info = mi }; return mb.Alloc(); } @@ -61,6 +60,7 @@ public static NewReference tp_repr(BorrowedReference op) protected override void OnClear() { target?.Dispose(); + targetType.Dispose(); target = null; } }