Fix MethodBinding/OverloadMapper memory leak (pythonnet#691) (pythonnet#2719) (#117)

* Fix MethodBinding/OverloadMapper memory leak (pythonnet#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.

* 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.

* 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

---------


(cherry picked from commit ca323cc)

Co-authored-by: greateggsgreg <36009512+greateggsgreg@users.noreply.github.com>
Co-authored-by: Benedikt Reinartz <filmor@gmail.com>

Author: 3 people

src/runtime/PythonTypes/PyObject.cs

@@ -93,6 +93,12 @@ internal PyObject(in StolenReference reference)
             Finalizer.Instance.ThrottledCollect();
         }
 
+        /// <summary>
+        /// Create a new PyObject instance of this object, bumping the reference
+        /// count.
+        /// </summary>
+        public PyObject NewReference() => new(this);
+
         // Ensure that encapsulated Python object is decref'ed appropriately
         // when the managed wrapper is garbage-collected.
         ~PyObject()

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");
         }
 
+        /// <summary>
+        /// Create a new PyType instance of this object, bumping the reference
+        /// count.
+        /// </summary>
+        public new PyType NewReference() => new(this);
+
         protected PyType(SerializationInfo info, StreamingContext context) : base(info, context) { }
 
         internal new static PyType? FromNullableReference(BorrowedReference reference)

src/runtime/Types/ExtensionType.cs

@@ -79,8 +79,18 @@ public unsafe static void tp_dealloc(NewReference lastRef)
             DecrefTypeAndFree(lastRef.Steal());
         }
 
+        /// <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() { }
+
         public static int tp_clear(BorrowedReference ob)
         {
+            (GetManagedObject(ob) as ExtensionType)?.OnClear();
+
             var weakrefs = Runtime.PyObject_GetWeakRefList(ob);
             if (weakrefs != null)
             {

src/runtime/Types/MethodBinding.cs

@@ -20,14 +20,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;
         }
@@ -64,7 +62,7 @@ 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?.NewReference(), self.targetType.NewReference());
             return mb.Alloc();
         }
 
@@ -151,7 +149,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);
+                    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;
@@ -261,7 +259,6 @@ public static NewReference tp_call(BorrowedReference ob, BorrowedReference args,
             }
         }
 
-
         /// <summary>
         /// MethodBinding  __hash__ implementation.
         /// </summary>
@@ -293,5 +290,12 @@ 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();
+            targetType.Dispose();
+            target = null;
+        }
     }
 }

src/runtime/Types/MethodObject.cs

@@ -226,8 +226,8 @@ public static NewReference tp_descr_get(BorrowedReference ds, BorrowedReference
                 && obj.inst is IPythonDerivedType
                 && self.type.Value.IsInstanceOfType(obj.inst))
             {
-                var basecls = ClassManager.GetClass(self.type.Value);
-                return new MethodBinding(self, new PyObject(ob), basecls).Alloc();
+                var basecls = ReflectedClrType.GetOrCreate(self.type.Value);
+                return new MethodBinding(self, new PyObject(ob), basecls.NewReference()).Alloc();
             }
 
             return new MethodBinding(self, target: new PyObject(ob), targetType: new PyType(tp)).Alloc();

src/runtime/Types/OverloadMapper.cs

@@ -9,12 +9,14 @@ namespace Python.Runtime
     /// </summary>
     internal class OverloadMapper : ExtensionType
     {
-        private MethodObject m;
+        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,7 +44,7 @@ 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?.NewReference(), self.targetType.NewReference()) { info = mi };
             return mb.Alloc();
         }
 
@@ -54,5 +56,12 @@ public static NewReference tp_repr(BorrowedReference op)
             var self = (OverloadMapper)GetManagedObject(op)!;
             return self.m.GetDocString();
         }
+
+        protected override void OnClear()
+        {
+            target?.Dispose();
+            targetType.Dispose();
+            target = null;
+        }
     }
 }

tests/test_method.py

@@ -961,8 +961,10 @@ def test_getting_generic_method_binding_does_not_leak_memory():
     bytesAllocatedPerIteration = pow(2, 20)  # 1MB
     bytesLeakedPerIteration = processBytesDelta / iterations
 
-    # Allow 50% threshold - this shows the original issue is fixed, which leaks the full allocated bytes per iteration
-    failThresholdBytesLeakedPerIteration = bytesAllocatedPerIteration / 2
+    # 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
 
@@ -1005,8 +1007,8 @@ def test_getting_overloaded_method_binding_does_not_leak_memory():
     bytesAllocatedPerIteration = pow(2, 20)  # 1MB
     bytesLeakedPerIteration = processBytesDelta / iterations
 
-    # Allow 50% threshold - this shows the original issue is fixed, which leaks the full allocated bytes per iteration
-    failThresholdBytesLeakedPerIteration = bytesAllocatedPerIteration / 2
+    # Tight 10% threshold; see test_getting_generic_method_binding_does_not_leak_memory.
+    failThresholdBytesLeakedPerIteration = bytesAllocatedPerIteration * 0.1
 
     assert bytesLeakedPerIteration < failThresholdBytesLeakedPerIteration
 
@@ -1049,8 +1051,8 @@ def test_getting_method_overloads_binding_does_not_leak_memory():
     bytesAllocatedPerIteration = pow(2, 20)  # 1MB
     bytesLeakedPerIteration = processBytesDelta / iterations
 
-    # Allow 50% threshold - this shows the original issue is fixed, which leaks the full allocated bytes per iteration
-    failThresholdBytesLeakedPerIteration = bytesAllocatedPerIteration / 2
+    # Tight 10% threshold; see test_getting_generic_method_binding_does_not_leak_memory.
+    failThresholdBytesLeakedPerIteration = bytesAllocatedPerIteration * 0.1
 
     assert bytesLeakedPerIteration < failThresholdBytesLeakedPerIteration