Skip to content

Alternative attempt at fixing memleak by being explicit about copies #2722

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Closed
filmor wants to merge 3 commits into from
Closed

Alternative attempt at fixing memleak by being explicit about copies #2722

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
a305127
Fix MethodBinding/OverloadMapper memory leak (#691)
greateggsgreg May 9, 2026
7f8a247
Tighten leak-test threshold to 10% to actually fail the bug
greateggsgreg May 9, 2026
11bebe4
Bugfix and improvements
filmor May 11, 2026
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
6 changes: 6 additions & 0 deletions src/runtime/PythonTypes/PyObject.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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()
Expand Down
6 changes: 6 additions & 0 deletions src/runtime/PythonTypes/PyType.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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)
Expand Down
10 changes: 10 additions & 0 deletions src/runtime/Types/ExtensionType.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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)
{
Expand Down
20 changes: 12 additions & 8 deletions src/runtime/Types/MethodBinding.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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;
}
Expand Down Expand Up @@ -54,7 +52,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();
}

Expand Down Expand Up @@ -141,7 +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);
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;
Expand Down Expand Up @@ -249,7 +247,6 @@ public static NewReference tp_call(BorrowedReference ob, BorrowedReference args,
}
}


/// <summary>
/// MethodBinding __hash__ implementation.
/// </summary>
Expand Down Expand Up @@ -281,5 +278,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;
}
}
}
2 changes: 1 addition & 1 deletion src/runtime/Types/MethodObject.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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();
Expand Down
15 changes: 12 additions & 3 deletions src/runtime/Types/OverloadMapper.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -10,11 +10,13 @@ namespace Python.Runtime
internal class OverloadMapper : ExtensionType
{
private readonly MethodObject m;
private readonly PyObject? target;
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;
}

Expand Down Expand Up @@ -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();
}

Expand All @@ -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;
}
}
}
15 changes: 8 additions & 7 deletions tests/test_method.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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

Expand Down Expand Up @@ -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

Expand Down Expand Up @@ -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

Expand Down
Loading