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?

@Jacek I would have SomeObject implement IDisposable instead as other classes can also contain a reference to it, therefore when its no longer required it can be disposed of and collected by GC — Master Yoda, Nov 21 '17 at 13:32
True, so you can implement IDisposable in SomeObject and make use of this answer https://stackoverflow.com/a/447960/2811109 — Jacek, Nov 21 '17 at 13:39
I see "C++ mindset" in this question. It is _references_ and not _pointers_ in C# and while they have similarities they are different concepts. When an object is moved as a result of garbage collection the internal pointer is adjusted while the reference stays the same. Also, a C# finalizer is not the same as C++ destructor. Finalizers are used to release _unmanaged_ resource. This is quite rare unless you interface directly with the underlying platform/OS. The memory is handled by the garbage collector but to achieve any other cleanup that a C++ destructor would do you can use `IDisposable`. — Martin Liversage, Nov 21 '17 at 14:21

score 5 · Accepted Answer · answered Nov 21 '17 at 13:49

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:

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
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

You are right, my solution didn't actually do what it was supposed to... thank you for the super detailed reply! — user884248, Nov 21 '17 at 14:22

score 1 · Answer 2 · answered Nov 21 '17 at 14:04

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?

Arnel · Answer 3 · 2018-01-15T04:41:55.620

You can unsubscribe from the event on the Dispose method if your class implement the IDisposable interface, then you will have to call the Dispose method on the instance. The problem with this approach is that most of the time we don't know exactly when to unsubscribe (or when to call Dispose), if this is the case, I recommend you to use the Weak Event Pattern.

The weak event pattern uses Weak References from the source object to the listener so this relationship doesn’t stop the GC from collecting these objects. Most of the controls in WPF already implement the weak event pattern. One of the approaches to use it, is through the generic WeakEventManager class.

Take a look to this article to see some code samples that use the Weak Event Pattern.

C#: how to avoid this potential memory leak

3 Answers3