Skip to content

Commit ca323cc

Browse files
greateggsgregfilmor
greateggsgreg
and
filmor
authored
Fix MethodBinding/OverloadMapper memory leak (#691) (#2719)
* 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. * 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 --------- Co-authored-by: Benedikt Reinartz <filmor@gmail.com>
1 parent 25e0ccf commit ca323cc

7 files changed

Lines changed: 55 additions & 19 deletions

File tree

‎src/runtime/PythonTypes/PyObject.cs‎

Lines changed: 6 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -93,6 +93,12 @@ internal PyObject(in StolenReference reference)
9393
Finalizer.Instance.ThrottledCollect();
9494
}
9595

96+
/// <summary>
97+
/// Create a new PyObject instance of this object, bumping the reference
98+
/// count.
99+
/// </summary>
100+
public PyObject NewReference() => new(this);
101+
96102
// Ensure that encapsulated Python object is decref'ed appropriately
97103
// when the managed wrapper is garbage-collected.
98104
~PyObject()

‎src/runtime/PythonTypes/PyType.cs‎

Lines changed: 6 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -35,6 +35,12 @@ internal PyType(in StolenReference reference, bool prevalidated = false) : base(
3535
throw new ArgumentException("object is not a type");
3636
}
3737

38+
/// <summary>
39+
/// Create a new PyType instance of this object, bumping the reference
40+
/// count.
41+
/// </summary>
42+
public new PyType NewReference() => new(this);
43+
3844
protected PyType(SerializationInfo info, StreamingContext context) : base(info, context) { }
3945

4046
internal new static PyType? FromNullableReference(BorrowedReference reference)

‎src/runtime/Types/ExtensionType.cs‎

Lines changed: 10 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -84,8 +84,18 @@ public unsafe static void tp_dealloc(NewReference lastRef)
8484
DecrefTypeAndFree(lastRef.Steal());
8585
}
8686

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+
8795
public static int tp_clear(BorrowedReference ob)
8896
{
97+
(GetManagedObject(ob) as ExtensionType)?.OnClear();
98+
8999
var weakrefs = Runtime.PyObject_GetWeakRefList(ob);
90100
if (weakrefs != null)
91101
{

‎src/runtime/Types/MethodBinding.cs‎

Lines changed: 12 additions & 8 deletions
Original file line numberDiff line numberDiff line change
@@ -18,14 +18,12 @@ internal class MethodBinding : ExtensionType
1818
internal MaybeMethodInfo info;
1919
internal MethodObject m;
2020
internal PyObject? target;
21-
internal PyType? targetType;
21+
internal PyType targetType;
2222

23-
public MethodBinding(MethodObject m, PyObject? target, PyType? targetType = null)
23+
public MethodBinding(MethodObject m, PyObject? target, PyType targetType)
2424
{
2525
this.target = target;
26-
27-
this.targetType = targetType ?? target?.GetPythonType();
28-
26+
this.targetType = targetType;
2927
this.info = null;
3028
this.m = m;
3129
}
@@ -54,7 +52,7 @@ public static NewReference mp_subscript(BorrowedReference tp, BorrowedReference
5452
}
5553

5654
MethodObject overloaded = self.m.WithOverloads(overloads);
57-
var mb = new MethodBinding(overloaded, self.target, self.targetType);
55+
var mb = new MethodBinding(overloaded, self.target?.NewReference(), self.targetType.NewReference());
5856
return mb.Alloc();
5957
}
6058

@@ -141,7 +139,7 @@ public static NewReference tp_getattro(BorrowedReference ob, BorrowedReference k
141139
// FIXME: deprecate __overloads__ soon...
142140
case "__overloads__":
143141
case "Overloads":
144-
var om = new OverloadMapper(self.m, self.target);
142+
var om = new OverloadMapper(self.m, self.target?.NewReference(), self.targetType.NewReference());
145143
return om.Alloc();
146144
case "__signature__" when Runtime.InspectModule is not null:
147145
var sig = self.Signature;
@@ -249,7 +247,6 @@ public static NewReference tp_call(BorrowedReference ob, BorrowedReference args,
249247
}
250248
}
251249

252-
253250
/// <summary>
254251
/// MethodBinding __hash__ implementation.
255252
/// </summary>
@@ -281,5 +278,12 @@ public static NewReference tp_repr(BorrowedReference ob)
281278
string name = self.m.name;
282279
return Runtime.PyString_FromString($"<{type} method '{name}'>");
283280
}
281+
282+
protected override void OnClear()
283+
{
284+
target?.Dispose();
285+
targetType.Dispose();
286+
target = null;
287+
}
284288
}
285289
}

‎src/runtime/Types/MethodObject.cs‎

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -197,7 +197,7 @@ public static NewReference tp_descr_get(BorrowedReference ds, BorrowedReference
197197
&& self.type.Value.IsInstanceOfType(obj.inst))
198198
{
199199
var basecls = ReflectedClrType.GetOrCreate(self.type.Value);
200-
return new MethodBinding(self, new PyObject(ob), basecls).Alloc();
200+
return new MethodBinding(self, new PyObject(ob), basecls.NewReference()).Alloc();
201201
}
202202

203203
return new MethodBinding(self, target: new PyObject(ob), targetType: new PyType(tp)).Alloc();

‎src/runtime/Types/OverloadMapper.cs‎

Lines changed: 12 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -10,11 +10,13 @@ namespace Python.Runtime
1010
internal class OverloadMapper : ExtensionType
1111
{
1212
private readonly MethodObject m;
13-
private readonly PyObject? target;
13+
private PyObject? target;
14+
readonly PyType targetType;
1415

15-
public OverloadMapper(MethodObject m, PyObject? target)
16+
public OverloadMapper(MethodObject m, PyObject? target, PyType targetType)
1617
{
1718
this.target = target;
19+
this.targetType = targetType;
1820
this.m = m;
1921
}
2022

@@ -42,7 +44,7 @@ public static NewReference mp_subscript(BorrowedReference tp, BorrowedReference
4244
return Exceptions.RaiseTypeError(e);
4345
}
4446

45-
var mb = new MethodBinding(self.m, self.target) { info = mi };
47+
var mb = new MethodBinding(self.m, self.target?.NewReference(), self.targetType.NewReference()) { info = mi };
4648
return mb.Alloc();
4749
}
4850

@@ -54,5 +56,12 @@ public static NewReference tp_repr(BorrowedReference op)
5456
var self = (OverloadMapper)GetManagedObject(op)!;
5557
return self.m.GetDocString();
5658
}
59+
60+
protected override void OnClear()
61+
{
62+
target?.Dispose();
63+
targetType.Dispose();
64+
target = null;
65+
}
5766
}
5867
}

‎tests/test_method.py‎

Lines changed: 8 additions & 7 deletions
Original file line numberDiff line numberDiff line change
@@ -983,9 +983,10 @@ def test_getting_generic_method_binding_does_not_leak_memory(memory_usage_tracki
983983
bytesAllocatedPerIteration = pow(2, 20) # 1MB
984984
bytesLeakedPerIteration = processBytesDelta / iterations
985985

986-
# Allow 90% threshold - this shows the original issue is fixed, which leaks the full allocated bytes per iteration
987-
# Increased from 50% to ensure that it works on Windows with Python >3.13
988-
failThresholdBytesLeakedPerIteration = bytesAllocatedPerIteration * 0.9
986+
# Tight 10% threshold: with the fix the per-iteration leak is essentially
987+
# zero, while the bug retains the bulk of the 1 MB payload (~600 KB/iter
988+
# on 3.14 GIL). 100 KB/iter cleanly distinguishes the two states.
989+
failThresholdBytesLeakedPerIteration = bytesAllocatedPerIteration * 0.1
989990

990991
assert bytesLeakedPerIteration < failThresholdBytesLeakedPerIteration
991992

@@ -1025,8 +1026,8 @@ def test_getting_overloaded_method_binding_does_not_leak_memory(memory_usage_tra
10251026
bytesAllocatedPerIteration = pow(2, 20) # 1MB
10261027
bytesLeakedPerIteration = processBytesDelta / iterations
10271028

1028-
# Allow 90% threshold - this shows the original issue is fixed, which leaks the full allocated bytes per iteration
1029-
failThresholdBytesLeakedPerIteration = bytesAllocatedPerIteration * 0.9
1029+
# Tight 10% threshold; see test_getting_generic_method_binding_does_not_leak_memory.
1030+
failThresholdBytesLeakedPerIteration = bytesAllocatedPerIteration * 0.1
10301031

10311032
assert bytesLeakedPerIteration < failThresholdBytesLeakedPerIteration
10321033

@@ -1068,8 +1069,8 @@ def test_getting_method_overloads_binding_does_not_leak_memory(memory_usage_trac
10681069
bytesAllocatedPerIteration = pow(2, 20) # 1MB
10691070
bytesLeakedPerIteration = processBytesDelta / iterations
10701071

1071-
# Allow 90% threshold - this shows the original issue is fixed, which leaks the full allocated bytes per iteration
1072-
failThresholdBytesLeakedPerIteration = bytesAllocatedPerIteration * 0.9
1072+
# Tight 10% threshold; see test_getting_generic_method_binding_does_not_leak_memory.
1073+
failThresholdBytesLeakedPerIteration = bytesAllocatedPerIteration * 0.1
10731074

10741075
assert bytesLeakedPerIteration < failThresholdBytesLeakedPerIteration
10751076

0 commit comments

Comments
 (0)