Fix MethodBinding/OverloadMapper memory leak (#691)

greateggsgreg · greateggsgreg · commit a9844824240f · 2026-05-09T16:25:40.000-04:00
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.
diff --git 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());
         }
 
+        /// <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)
             {
diff --git 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
@@ -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;
+        }
     }
 }

Original file line number	Diff line number	Diff line change
`@@ -84,8 +84,18 @@ public unsafe static void tp_dealloc(NewReference lastRef)`
`84`	`84`	`DecrefTypeAndFree(lastRef.Steal());`
`85`	`85`	`}`
`86`	`86`
	`87`	`+ /// <summary>`
	`88`	`+ /// Called during tp_clear before the GCHandle is released.`
	`89`	`+ /// Override to eagerly dispose Python object references (PyObject fields)`
	`90`	`+ /// held by the subclass, preventing the multi-hop .NET finalizer chain`
	`91`	`+ /// from delaying Python-side refcount decrements.`
	`92`	`+ /// </summary>`
	`93`	`+ protected virtual void OnClear() { }`
	`94`	`+`
`87`	`95`	`public static int tp_clear(BorrowedReference ob)`
`88`	`96`	`{`
	`97`	`+ (GetManagedObject(ob) as ExtensionType)?.OnClear();`
	`98`	`+`
`89`	`99`	`var weakrefs = Runtime.PyObject_GetWeakRefList(ob);`
`90`	`100`	`if (weakrefs != null)`
`91`	`101`	`{`
Original file line number	Diff line number	Diff line change
`@@ -10,7 +10,7 @@ namespace Python.Runtime`
`10`	`10`	`internal class OverloadMapper : ExtensionType`
`11`	`11`	`{`
`12`	`12`	`private readonly MethodObject m;`
`13`		`- private readonly PyObject? target;`
	`13`	`+ private PyObject? target;`
`14`	`14`
`15`	`15`	`public OverloadMapper(MethodObject m, PyObject? target)`
`16`	`16`	`{`
`@@ -42,7 +42,10 @@ public static NewReference mp_subscript(BorrowedReference tp, BorrowedReference`
`42`	`42`	`return Exceptions.RaiseTypeError(e);`
`43`	`43`	`}`
`44`	`44`
`45`		`- var mb = new MethodBinding(self.m, self.target) { info = mi };`
	`45`	`+ var mb = new MethodBinding(`
	`46`	`+ self.m,`
	`47`	`+ self.target is null ? null : new PyObject(self.target.Reference))`
	`48`	`+ { info = mi };`
`46`	`49`	`return mb.Alloc();`
`47`	`50`	`}`
`48`	`51`
`@@ -54,5 +57,11 @@ public static NewReference tp_repr(BorrowedReference op)`
`54`	`57`	`var self = (OverloadMapper)GetManagedObject(op)!;`
`55`	`58`	`return self.m.GetDocString();`
`56`	`59`	`}`
	`60`	`+`
	`61`	`+ protected override void OnClear()`
	`62`	`+ {`
	`63`	`+ target?.Dispose();`
	`64`	`+ target = null;`
	`65`	`+ }`
`57`	`66`	`}`
`58`	`67`	`}`