Anonymous event handlers and disposing

Question

I have a rather short question about anonymous event handlers:

This is the code that I have:

public void AddTestControl(Control ctrl)
{
    ctrl.Disposed += (o, e) => { RemoveTestControl(ctrl); };
    ctrl.SomeEvent += _Control_SomeEvent;
}

public void RemoveTestControl(Control ctrl)
{
    ctrl.SomeEvent -= _Control_SomeEvent;
}

Is this code above fine, or should the code be rewritten in order to remove the Disposed Event Handler? Something like this:

public void AddTestControl(Control ctrl)
{
    ctrl.Disposed += _Control_Disposed;
    ctrl.SomeEvent += _Control_SomeEvent;
}

public void RemoveTestControl(Control ctrl)
{
    ctrl.Disposed -= _Control_Disposed;
    ctrl.SomeEvent -= _Control_SomeEvent;
}

Enrico Campidoglio · Accepted Answer · 2012-03-05T11:18:29.333

8

Generally, the only situation where you need to remove the event handlers from an object in order for it to be eligible for garbage collection is when the publisher object (the one defining the event) lives longer than the subscriber object (the one containing the event handlers). In such a situation the GC won't be able to release the subscriber when it goes out of scope since it's still being referenced by the publisher.

In this case, assuming this is WebForms or WinForms, the publisher (that is the Control object), is most likely a child object of the subscriber (probably a Page or a Form), which will be the first to go out of scope taking all its associated objects with it. Hence there's no need to remove the event handlers.

edited Mar 05 '12 at 11:18

answered Mar 02 '12 at 10:11

Enrico Campidoglio

47,702
11
112
146

afaik nobody promises execution order of event handlers. – b0rg Mar 02 '12 at 10:36
But my question is more about the Disposed event. Do I need to detach it myself or is the first code sample just fine? – juFo Mar 02 '12 at 10:36
1

@juFo Assuming that the reason you ask is because you're worried about memory leaks, then **no, you don't need to detach the event handlers yourself**. Since the `Control` object has the same lifetime of the `Page` or `Form` containing the event handlers, they will go out of scope together. – Enrico Campidoglio Mar 02 '12 at 10:45
You need to switch the words "publisher" and "subscriber" in your answer though. It's publishers which hold references to subscribers (in the invocation list of the delegate backing the event), hence long-lived publishers which keep subscribers alive unnecessarily. – anton.burger Mar 05 '12 at 09:33
@Anton You're absolutely correct. Thanks for pointing that out. – Enrico Campidoglio Mar 05 '12 at 11:20

score 2 · Answer 2 · answered Mar 02 '12 at 10:38

It always feels cleaner to me to unsubscribe from events, even in situations where I know the subscribers always outlive the publisher (the object raising the event): the event will never be raised again, the publisher is no longer reachable and can be collected.

Then again, how many people go to the trouble of unsubscribing every event handler in e.g. a WinForms application? The object references point from the publisher to the subscribers, not the other way around, so the publisher can be collected while the subscribers live on. It doesn't present the same danger as the opposite situation, where a long-lived publisher (e.g. a static event) can wastefully keep potentially large subscribers alive long after they could have been collected.

If you want/need to unsubscribe, then the requirement to unsubscribe the same delegate makes anonymous event handlers a bit of a pain. The Reactive Extensions solve this problem in a neat way: rather than having to remember the delegate you subscribed, subscribing returns an IDisposable which unsubscribes when disposed. Chucking all your subscriptions into a CompositeDisposable lets you unsubscribe everything with just one Dispose call.

score 1 · Answer 3 · answered Mar 02 '12 at 10:45

Both codez are fine, but I like second as a matter of personal preference. It reads clearer than the first.

On top of it with the first code, there's anonymous lambda delegate and it gets a current reference to ctrl. That code may behave unexpectedly depending on situation and compile optimisation settings: whether or not the call is inlined or not.

Not to mention that there's architectural problem with the code: you have ControlOwner and a bunch of Child Controls. I take it you're adding child controls to ControlOwner at runtime, and then trying to react to their behavivour by subscribing ControlOwner to the childControl events. This is fine for events like _ButtonClicked etc. But not good for Dispose. Let the child control handle it's disposing on itself, the OwnerControl doesn't need to know about it. Not to mention that it may not exist at a time ChildControl[n].Dispose is called.

In short: * it is better to leave dispose event on ChildControl alone and do all clean up in ChildControl.Dispose * it is not necessary to unsubscribe from events. Event dispatcher will check if subscriber is alive.

Anonymous event handlers and disposing

3 Answers3

Linked