Repository navigation
Commit a305127
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.1 parent 25e0ccf commit a305127
3 files changed
Lines changed: 33 additions & 4 deletions
| 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 | |
|---|---|---|---|
@@ -54,7 +54,10 @@ public static NewReference mp_subscript(BorrowedReference tp, BorrowedReference | |||
| 54 | 54 | } | |
| 55 | 55 | ||
| 56 | 56 | MethodObject overloaded = self.m.WithOverloads(overloads); | |
| 57 | - var mb = new MethodBinding(overloaded, self.target, self.targetType); | ||
| 57 | + var mb = new MethodBinding( | ||
| 58 | + overloaded, | ||
| 59 | + self.target is null ? null : new PyObject(self.target.Reference), | ||
| 60 | + self.targetType is null ? null : new PyType(self.targetType.Reference)); | ||
| 58 | 61 | return mb.Alloc(); | |
| 59 | 62 | } | |
| 60 | 63 | ||
@@ -141,7 +144,8 @@ public static NewReference tp_getattro(BorrowedReference ob, BorrowedReference k | |||
| 141 | 144 | // FIXME: deprecate __overloads__ soon... | |
| 142 | 145 | case "__overloads__": | |
| 143 | 146 | case "Overloads": | |
| 144 | - var om = new OverloadMapper(self.m, self.target); | ||
| 147 | + var om = new OverloadMapper(self.m, | ||
| 148 | + self.target is null ? null : new PyObject(self.target.Reference)); | ||
| 145 | 149 | return om.Alloc(); | |
| 146 | 150 | case "__signature__" when Runtime.InspectModule is not null: | |
| 147 | 151 | var sig = self.Signature; | |
@@ -281,5 +285,11 @@ public static NewReference tp_repr(BorrowedReference ob) | |||
| 281 | 285 | string name = self.m.name; | |
| 282 | 286 | return Runtime.PyString_FromString($"<{type} method '{name}'>"); | |
| 283 | 287 | } | |
| 288 | + | ||
| 289 | + protected override void OnClear() | ||
| 290 | + { | ||
| 291 | + target?.Dispose(); | ||
| 292 | + target = null; | ||
| 293 | + } | ||
| 284 | 294 | } | |
| 285 | 295 | } | |
| 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 | } | |
0 commit comments