What best practices for cleaning up event handler references?

user_72871 0
Viewed 38572

Often I find myself writing code like this:

        if (Session != null)
        {
            Session.KillAllProcesses();
            Session.AllUnitsReady -= Session_AllUnitsReady;
            Session.AllUnitsResultsPublished -= Session_AllUnitsResultsPublished;
            Session.UnitFailed -= Session_UnitFailed;
            Session.SomeUnitsFailed -= Session_SomeUnitsFailed;
            Session.UnitCheckedIn -= Session_UnitCheckedIn;
            UnattachListeners();
        }

The purpose being to clean up all event subscriptions that we have registered for on the target (Session) so that Session is free to be disposed by the GC. I had a discussion with a co-worker about classes that implement IDisposable however and it was his belief that those classes should preform cleanup like this:

    /// <summary>
    /// Disposes the object
    /// </summary>
    public void Dispose()
    {
        SubmitRequested = null; //frees all references to the SubmitRequested Event
    }

Is there a reason for prefering one over the other? Is there a better way to go about this altogether? (Aside from weak reference events everywhere)

What I'd really like to see is somethign akin to the safe invocation pattern for raising events: i.e. safe and repeatable. Something I can remember to do everytime I attach to an event so that I can ensure it will be easy for me to clean up.

dispose garbage-collection events c#
6 Answers

The answer from DanH is almost there, but it's missing one crucial element.

For this to always work properly, must first take a local copy of the variable, in case it changes. Essentially, we have to enforce an implicitly captured closure.

List<IDisposable> eventsToDispose = new List<IDisposable>();

var handlerCopy = this.ViewModel.Selection;
eventsToDispose.Add(Disposable.Create(() => 
{
    handlerCopy.CollectionChanged -= SelectionChanged;
}));

And later on, we can dispose of all events using this:

foreach(var d in eventsToDispose)
{ 
    d.Dispose();
}

If we want to make it shorter:

eventsToDispose.ForEach(o => o.Dispose());

If we want to make it even shorter, we can replace IList with CompositeDisposable, which is exactly the same thing behind the scenes.

Then we can dispose of all events with this:

eventsToDispose.Dispose();
user_1074090
Related