Do you need to remove an event handler in the destructor?

Question

I use some UserControls which get created and destroyed within my application during runtime (by creating and closing subwindows with these controls inside).
It's a WPF UserControl and inherits from System.Windows.Controls.UserControl. There is no Dispose() method I could override.
PPMM is a Singleton with the same lifetime as my application.
Now in the constructor of my (WPF) UserControl, I add an event handler:

public MyControl()
{
    InitializeComponent();

    // hook up to an event
    PPMM.FactorChanged += new ppmmEventHandler(PPMM_FactorChanged);
}

I got used to removing such event handler in the destructor:

~MyControl()
{
    // hook off of the event
    PPMM.FactorChanged -= new ppmmEventHandler(PPMM_FactorChanged);
}

Today I stumbled upon this and wondered:

1) Is this neccessary? Or does the GC take care of it?

2) Does this even work? Or would I have to store the newly created ppmmEventHandler?

I'm looking forward to your answers.

Answer 1

Firstly I would say do not use a destructor but Dispose() to clear your resources.

Secondly, in my opinion, if this code is inside an object that is created very often and has a short lifetime, it's better to take care of removing the event handler yourself as this is a link to the holder object, which will prevent the GC from collecting it.

Regards.

Answer 2

Since PPMM is a long-lived object (singleton), then this code doesn't make much sense.

The problem here is that as long as that event handler is referencing the object, it will not be eligible for garbage collection, as least as long as that other object that owns the event is alive.

As such, putting anything in the destructor is pointless, as either:

1. The event handler has already been removed, thus the object became eligible for garbage collection
2. The event handler is not removed, the owning object is not eligible for garbage collection, and thus the finalizer will never get called
3. Both objects are eligible for garbage collection, in which case you should not access that other object at all in the finalizer since you don't know its internal state

In short, don't do this.

Now, a different argument could be said about adding such code to the Dispose method, when you're implementing IDisposable. In that case it fully makes sense since its usercode that is calling Dispose, at a predefined and controlled point.

The finalizer (destructor), however, is only called when the object is eligible for garbage collection and has a finalizer, in which case there is no point.

As for question nbr. 2, which I take as "Can I unsubscribe from events like that", then yes, you can. The only time you need to hold on to the delegate you used to subscribe with is when you're constructing the delegate around an anonymous method or a lambda expression. When you're constructing it around an existing method, it will work.

---

Edit: WPF. right, didn't see that tag. Sorry, the rest of my answer doesn't make much sense for WPF and since I am no WPF-guru, I can't really say. However, there's a way to fix this. It's entirely legal here on SO to poach the content of another answer if you can improve it. So if anyone knows how to properly do this with a WPF usercontrol, you're free to lift the entire first section of my answer and add the relevant bits of WPF.

Edit: Let me respond to the question in the comment inside here as well.

Since the class in question is a user-control, its lifetime will be tied to a form. When the form is closing, it will dispose of all child controls that it owns, in other words, there is already a Dispose method present here.

The correct way for a user control to handle this, if it manages its own events, is to unhook the event handlers in the Dispose method.

(rest removed)

Answer 3

WPF doesn't support IDisposable well. If you're implementing a WPF control that needs cleanup, you should think about hooking into the Loaded and Unloaded events instead (or in addition).

I.e. you connect to the event in the Loaded handler and disconnect in the Unloaded handler. Of course this is only an option if your control does not need to receive the event while it's not "loaded" and if you can correctly support many load/unload cycles.

The advantage of using the Loaded/Unloaded events is that you don't have to manually dispose the user control everywhere it's used. You should however be aware that the Unloaded event is not fired after application shutdown has begun. E.g. if your shutdown mode is OnMainWindowClose, the Unloaded events for other windows will not be fired. This usually isn't a problem though. It just means that you cannot do stuff reliably in an Unloaded that must happen before/while the application terminates.

Answer 4

If the code got to the destructor, it doesn't matter anymore.

That's because it will only be destroyed if it isn't listening to any events anymore.
If it was still listening to events, it wouldn't have been destroyed.

Answer 5

Is PPMM something external with a longer lifetime that the MyControl instances?

If so, unless PPMM_FactorChanged is a static method, the ppmmEventHandler will be keeping a reference to the MyControl instance live - which means the instance will never be eligible for garbage collection, and the finalizer will never fire.

You shouldn't need to keep the ppmmEventHandler around for the removal code.

Answer 6

The GC will take care of that. While the event holds a strong reference, it only holds it on the parent object itself. In the end only MyControl will keep a reference via the event handler, thus the GC will collect it.

On the otherhand using the finalizer, which is NOT a descrutor. Is for this bad practice. If you want to unregister an event, you should consider IDisposable.

Answer 7

Event handlers are tricky and can easily hide a resource leak. As Tigran says. Use IDisposeable and forget about destructors. I recommend measuring whether you got it right. Just by looking at the memory consumption of your app in task manager will tell if a leak is present if you stress test it a little bit by loading up and closing a few thousand windows.

Answer 8

There are some cases where unsubscribing from an event in a Finalizer/destructor could be useful, if the event publisher guaranteed that unsubscription was thread-safe. For an object to unsubscribe from its own events would be useless, but one could as a workable pattern have a public-facing object hold a reference to a private object that actually "does all the work", and have that private object subscribe to events. If there is no reference from the private object back to the public object, the public object will become eligible for finalization once nobody is still interested in it; its finalizer would then be able to cancel the subscription on behalf of the private object.

Unfortunately, this pattern can only work if the objects whose events are subscribed guarantees that it can accept unsubscription requests from any threading context, and not just the context where events were subscribed. Having .NET require as part of the "event" contract that all unsubscribe methods must be thread-safe would not have imposed a major burden on implementations, but for whatever reasons MS did not impose such a requirement. Consequently, even if a Finalizer/destructor discovers that an event should be unsubscribed as soon as possible, there is no standard mechanism by which it can make that happen.