C# Events Memory Leak [closed]

Question

When does these unsubscribed events memory leak occurs? Should I write destructor or implement IDisposable to unsubscribe an event?

Tami

Simple rule: Don't use finalizers. – Rawdin

Answer 1

Let's say that A references B. Furthermore, say you think you're done with B and expect it to be garbage collected.

Now, if A is reachable[1], B won't be garbage collected, despite the fact that "you're done with it". This is, in all essence, a memory leak[2]

If B subscribes to an event in A, then we have the same situation: A has a reference to B via the event handler delegate.

So, when is this a problem? Only when the referencing object is reachable, as mentioned above. In this case, there can be a leak when a Foo instance isn't used any longer:

class Foo
{
    Bar _bar;

    public Foo(Bar bar)
    {
        _bar = bar;
        _bar.Changed += BarChanged;
    }

    void BarChanged(object sender, EventArgs e) { }
}

The reason why there can be a leak is that the Bar instance passed in the constructor can have a longer lifetime than the Foo instance using it. The subscribed event handler can then keep the Foo alive.

In this case you need to provide a way to unsubscribe from the event to not get a memory leak. One way of doing that is by letting Foo implement IDisposable. The upside of that is that it clearly signals to the class consumer that he need to call Dispose() when done. Another way is to have separate Subscribe() and Unsubscribe() methods, but that doesn't convey the type's expectations - they are too optional to call and introduce a temporal coupling.

My recommendation is:

class sealed Foo : IDisposable
{
    readonly Bar _bar;
    bool _disposed;

    ...

    public void Dispose()
    {
        if (!_disposed)
        {
            _disposed = true;
            _bar.Changed -= BarChanged;
        }
    }

    ...
}

Or alternatively:

class sealed Foo : IDisposable
{
    Bar _bar;

    ...

    public void Dispose()
    {
        if (_bar != null)
        {
            _bar.Changed -= BarChanged;
            _bar = null;
        }
    }

    ...
}

On the other hand, when the referencing object isn't reachable, there can't be a leak:

class sealed Foo
{
    Bar _bar;

    public Foo()
    {
        _bar = new Bar();
        _bar.Changed += BarChanged;
    }

    void BarChanged(object sender, EventArgs e) { }
}

In this case any Foo instance will always outlive its composed Bar instance. When a Foo is unreachable, so will its Bar be. The subscribed event handler cannot keep the Foo alive here. The downside of this is that if Bar is a dependency in need of being mocked in unit testing scenarios, it can't (in any clean way) be explicitly instantiated by the consumer, but needs to be injected.

[1] http://msdn.microsoft.com/en-us/magazine/bb985010.aspx

[2] http://en.wikipedia.org/wiki/Memory_leak

Disturbed

In the case where the referencing object isn't reachable, don't you have a circular reference, which would keep both objects alive? A Foo has a reference to _bar and _bar has a reference to that Foo through it's handler. Am I wrong to think this is a circular reference? Is the C# GC smart enough to handle circular references and garbage collect non the less? Or do we actually still find ourselves with a memory leak? – Sogdiana

The circular references won't affect garbage collection. The reason is perfectly described here: https://mcmap.net/q/270969/-circular-references-cause-memory-leak – Disturbed

Answer 2

An event handler contains a strong reference to the object that declares the handler (in the delegate's Target property).

Until the event handler is removed (or until the object owning the event is no longer referenced anywhere), the object containing the handler will not be collected.

You can fix this by removing the handler when you no longer need it (perhaps in Dispose()).
A finalizer cannot help, because the finalizer would only run after it can be collected.

Rawdin

So shouldn't i remove all event handlers manually just for safety? – Tami

Depends on whether the object which declares the event is collected in time. This is not the case for a static event, so you should unregister your handler in the manner described. – Trinitarianism

@fex: You should take out the time to figure out how long each object will last and remove where necessary. If you want to produce a reliable program, it is very important that you understand how you want it to work. – Rawdin