C#: how to avoid this potential memory leak

Question

Let's assume I have a C# class which looks like this:

public class MyClass {
    public SomeObject TheObject { get; }

    public MyClass() {
        TheObject = new SomeObject();
        TheObject.MyEvent += MyEventHandler;
    }

    private void MyEventHandler() {
        // some code
    }
}

The class creates an internal object called TheObject of type SomeObject, and adds an event handler to an event on this object.

Since TheObject is a public property, that means that any other piece of code can maintain a pointer to this object; And this will, in turn, keep the object of type MyClass alive, because TheObject has a pointer to MyClass in the form of an event handler.

So, I assume the only way to keep this code safe from this event is to add a finalizer to MyClass:

public ~MyClass() {
    TheObject?.MyEvent -= MyEventHandler;
}

This is too bad because the finalizer will bump up objects of type MyClass to the next GC generation, but am I correct that this is the only way to avoid this potential memory leak?

Answer 1

Your solution will not actually fix the problem because the finalizer itself will not get called until the object can be collected, and as you correctly identified TheObject will keep the object alive through the event handler.

There are two potential fixes:

1. Make MyClass implement IDisposable and unregister the event handler in the Dispose method. C# has the using syntax to help with the usage of the class
2. Use weak references for the events and not rely on the default event syntax.

A simple implementation for this would be:

public interface ISubscription
{
    bool IsAlive { get; }
    void Fire();
}

public class Subscrition<T> : ISubscription
    where T: class
{
    public Subscrition(T target, Action<T> fire)
    {
        this.Target = new WeakReference<T>(target);
        this.FireHandler = fire;
    }
    public WeakReference<T> Target { get; }
    public Action<T> FireHandler { get; }

    public bool IsAlive => this.Target.TryGetTarget(out var t);

    public void Fire()
    {
        if (this.Target.TryGetTarget(out var target))
        {
            this.FireHandler(target);
        }
    }
}

public class WeakEvent
{
    List<ISubscription> weakHandlers = new List<ISubscription>();

    public void Register<T>(T target, Action<T> fire)
        where T:class
    {
        this.Prune();
        this.weakHandlers.Add(new Subscrition<T>(target, fire));
    }
    public void Unregister(ISubscription subscription)
    {
        this.Prune();
        this.weakHandlers.Remove(subscription);
    }
    // Prune any dead handlers.
    public void Prune()
    {
        this.weakHandlers.RemoveAll(_ => !_.IsAlive);
    }
    public void Fire()
    {
        this.Prune(); 
        this.weakHandlers.ForEach(_ => _.Fire());

    }
}

Usage:

public class SomeObject
{
    public WeakEvent WeakMyEvent = new WeakEvent();
}

public class MyClass
{
    public SomeObject TheObject { get; }

    public MyClass()
    {
        TheObject = new SomeObject();
        TheObject.WeakMyEvent.Register(this, t => t.MyEventHandler());
    }
    private void MyEventHandler()
    {
        // some code
    }
}

You could also checkout this article for a more complex implementation

Answer 2

you have to implement IDISPOSABLE , using -= to avoid memory leaks is useless, because your object is always alive. If your garbage collector is useless to make free your delegate. You have more information in the same subject: Why and How to avoid Event Handler memory leaks?