Skip to content

Delete target object from event handler collections when it has no more event handlers #1973

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.

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

Merged
merged 3 commits into from
Oct 14, 2022
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
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
2 changes: 2 additions & 0 deletions CHANGELOG.md
Original file line number Diff line number Diff line change
Expand Up @@ -13,6 +13,8 @@ This document follows the conventions laid out in [Keep a CHANGELOG][].

### Fixed

- Fixed objects leaking when Python attached event handlers to them even if they were later removed


## [3.0.0](https://github.com/pythonnet/pythonnet/releases/tag/v3.0.0) - 2022-09-29

Expand Down
67 changes: 67 additions & 0 deletions src/embed_tests/Events.cs
Original file line number Diff line number Diff line change
@@ -0,0 +1,67 @@
using System;
using System.Diagnostics;
using System.Threading;

using NUnit.Framework;

using Python.Runtime;

namespace Python.EmbeddingTest;

public class Events
{
[OneTimeSetUp]
public void SetUp()
{
PythonEngine.Initialize();
}

[OneTimeTearDown]
public void Dispose()
{
PythonEngine.Shutdown();
}

[Test]
public void UsingDoesNotLeak()
{
using var scope = Py.CreateScope();
scope.Exec(@"
import gc

from Python.EmbeddingTest import ClassWithEventHandler

def event_handler():
pass

for _ in range(2000):
example = ClassWithEventHandler()
example.LeakEvent += event_handler
example.LeakEvent -= event_handler
del example

gc.collect()
");
Runtime.Runtime.TryCollectingGarbage(10);
Assert.AreEqual(0, ClassWithEventHandler.alive);
}
}

public class ClassWithEventHandler
{
internal static int alive;

public event EventHandler LeakEvent;
private Array arr; // dummy array to exacerbate memory leak

public ClassWithEventHandler()
{
Interlocked.Increment(ref alive);
this.arr = new int[800];
}

~ClassWithEventHandler()
{
Interlocked.Decrement(ref alive);
}
}
2 changes: 2 additions & 0 deletions src/runtime/Finalizer.cs
Original file line number Diff line number Diff line change
Expand Up @@ -364,6 +364,8 @@ struct PendingFinalization
{
public IntPtr PyObj;
public BorrowedReference Ref => new(PyObj);
public ManagedType? Managed => ManagedType.GetManagedObject(Ref);
public nint RefCount => Runtime.Refcount(Ref);
public int RuntimeRun;
#if TRACE_ALLOC
public string StackTrace;
Expand Down
17 changes: 14 additions & 3 deletions src/runtime/PythonTypes/PyObject.cs
Original file line number Diff line number Diff line change
Expand Up @@ -1051,9 +1051,20 @@ public PyList Dir()
return Runtime.GetManagedString(strval.BorrowOrThrow());
}

string? DebuggerDisplay => DebugUtil.HaveInterpreterLock()
? this.ToString()
: $"pyobj at 0x{this.rawPtr:X} (get Py.GIL to see more info)";
ManagedType? InternalManagedObject => ManagedType.GetManagedObject(this.Reference);

string? DebuggerDisplay
{
get
{
if (DebugUtil.HaveInterpreterLock())
return this.ToString();
var obj = this.InternalManagedObject;
return obj is { }
? obj.ToString()
: $"pyobj at 0x{this.rawPtr:X} (get Py.GIL to see more info)";
}
}


/// <summary>
Expand Down
23 changes: 21 additions & 2 deletions src/runtime/Util/CodeGenerator.cs
Original file line number Diff line number Diff line change
@@ -1,5 +1,7 @@
using System;
using System.Collections.Generic;
using System.Globalization;
using System.Linq;
using System.Reflection;
using System.Reflection.Emit;
using System.Threading;
Expand All @@ -17,13 +19,15 @@ internal class CodeGenerator
private readonly AssemblyBuilder aBuilder;
private readonly ModuleBuilder mBuilder;

const string NamePrefix = "__Python_Runtime_Generated_";

internal CodeGenerator()
{
var aname = new AssemblyName { Name = "__CodeGenerator_Assembly" };
var aname = new AssemblyName { Name = GetUniqueAssemblyName(NamePrefix + "Assembly") };
var aa = AssemblyBuilderAccess.Run;

aBuilder = Thread.GetDomain().DefineDynamicAssembly(aname, aa);
mBuilder = aBuilder.DefineDynamicModule("__CodeGenerator_Module");
mBuilder = aBuilder.DefineDynamicModule(NamePrefix + "Module");
}

/// <summary>
Expand Down Expand Up @@ -77,5 +81,20 @@ internal static void GenerateMarshalByRefsBack(ILGenerator il, IReadOnlyList<Typ
}
}
}

static string GetUniqueAssemblyName(string name)
{
var taken = new HashSet<string>(AppDomain.CurrentDomain
.GetAssemblies()
.Select(a => a.GetName().Name));
for (int i = 0; i < int.MaxValue; i++)
{
string candidate = name + i.ToString(CultureInfo.InvariantCulture);
if (!taken.Contains(candidate))
return candidate;
}

throw new NotSupportedException("Too many assemblies");
}
}
}
4 changes: 4 additions & 0 deletions src/runtime/Util/EventHandlerCollection.cs
Original file line number Diff line number Diff line change
Expand Up @@ -99,6 +99,10 @@ internal bool RemoveEventHandler(BorrowedReference target, BorrowedReference han
continue;
}
list.RemoveAt(i);
if (list.Count == 0)
{
Remove(key);
}
return true;
}

Expand Down