Do I need to remove this sort of event handler?

Question

If I create a .NET class which subscribes to an event with an anonymous function like this:

void MyMethod()
{
   Application.Current.Deactivated += (s,e) => { ChangeAppearance(); };
}

Will this event handler be a root keeping my class from being garbage collected?

If not, wohoo! But if so, can you show me the removal syntax? Just using -= with the same code seems wrong.

Answer 1

You can either use a "real" method as Simon D. suggests, or do this:

EventHandler handler = null;
handler = (s,e) => { 
    Application.Current.Deactivated -= handler;
    ChangeAppearance();
};
Application.Current.Deactivated += handler;

Since this is somewhat ugly and defeats the purpose of using a lambda to be succint, I 'd probably go with refactoring it out to a method. But it's useful to know what else works.

Warning: It goes without saying that you have to be super ultra careful to not mess with the value of handler at all between the point where you subscribe to the event and the point where it is actually invoked, otherwise the unsubscribe part won't work correctly.

Answer 2

I think you do need to unsubscribe, because the event provider (the application) lives - or at least can live - longer than your consumer. So each subscribing instance dying while the app remains living will create memory leaks.

You are subscribing an anonymous delegate to the event. This is why you can't unsubscribe it the same way, because you cannot address it anymore. Actually you are creating the method at the same moment you subscribe, and you don't store any pointer to the newly created method.

If you slightly change your implementation to use a "real" method, you can easily unsubscribe from the event the same way you subscribe to it:

Application.Current.Deactivated += ChangeAppearance;
Application.Current.Deactivated -= ChangeAppearance;
private void ChangeAppearance(object sender, EventArgs eventArgs)
{
    throw new NotImplementedException();
}

Answer 3

You definitely have to clean up the reference. If there were any doubt you could easily test with your own static event like so.

static class MemoryLeak
{
    static List<Action<int>> list = new List<Action<int>>();
    public static event Action<int> ActivateLeak
    {
        add
        {
            list.Add(value);
        }
        remove
        {
            list.Remove(value);
        }
    }
}

Then by setting a breakpoint in the remove function you can see that your reference is not cleaned up.

class Program
{
    static void Main(string[] args)
    {
        foo f = new foo();
        MemoryLeak.ActivateLeak += o => f.bar();
        f.tryCleanup();
    }
}

class foo
{
    public void bar()
    { }

    public void tryCleanup()
    {
        MemoryLeak.ActivateLeak -= o => bar();
    }
}

As an alternative to Simon's solution you could use a second closure to create a "detach" action which can be passed around.

foo f = new foo();
Action<int> callfoo = o => f.bar();
MemoryLeak.ActivateLeak += callfoo;
Action cleanUp = () => MemoryLeak.ActivateLeak -= callfoo;

// Now you can pass around the cleanUp action and call it when you need to unsubscribe from the event.
cleanUp();

Answer 4

This is indeed a leak that prevents garbage collection. There are ways around it - with WeakReference being one of the better ones.

This link has a good conversation and good answers for you.