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.

Solution 1

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)

Solution 2

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.

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

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

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

View more solutions