What best practices for cleaning up event handler references?

Question

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.

Answer 1

Implementing IDisposable has two advantages over the manual method:

1. It's standard and the compiler treats it specially. Meaning that everybody that reads your code understand what it's about the minute they see IDisposable being implemented.
2. .NET C# and VB provide special constructs for working with IDisposable via the using statement.

Still, I doubt whether this is useful in your scenario. To safely dispose of an object, it needs to be disposed of in the finally-block inside a try/catch. In the case you seem to describe, it may require that either Session takes care of this, or the code calling the Session, upon deletion of the object (i.e., at the end of its scope: in the finally block). If so, the Session must implement IDisposable too, which follows the common concept. Inside the IDisposable.Dispose method, it loops through all its members that are disposable and disposes of them.

Edit

Your latest comment makes me rethink my answer and try to connect a few dots. You want to make sure Session is disposable by the GC. If the references to the delegates are from inside the same class, it is not necessary at all to unsubscribe them. If they are from another class, you need to unsubscribe them. Looking at the code above, you seem to write that code block in any class that uses Session and clean it up at some point in the process.

If Session needs to be freed, there's a more direct way were calling class needs not be responsible for correct handling the unsubscribe process. Simply loop all events using trivial reflection and set all to null (you can consider alternative approaches to reach the same effect).

Because you ask for "best practices", you should combine this method with IDisposable and implement the loop inside IDisposable.Dispose(). Before you enter this loop, you call one more event: Disposing, which listeners can use if they need to clean up anything themselves. When using IDisposable, be aware of its caveats, of which this briefly described pattern is a common solution.

Answer 2

It is incorrect to say that unregistering the handlers from the Session events will somehow allow a Session object to be collected by the GC. Here is a diagram that illustrates the reference chain of events.

--------------      ------------      ----------------
|            |      |          |      |              |
|Event Source|  ==> | Delegate |  ==> | Event Target |
|            |      |          |      |              |
--------------      ------------      ----------------

So in your case the event source is a Session object. But I do not see that you mentioned which class declared the handlers so we do not yet known who the event target is. Lets consider two possibilities. The event target could be the same Session object that represents the source or it could be an entirely separate class. In either case and under normal circumstances the Session will be collected as long as there is not another reference to even if the handlers to its events remain registered. That is because the delegate does not contain a reference back to the event source. It only contains a reference to the event target.

Consider the following code.

public static void Main()
{
  var test1 = new Source();
  test1.Event += (sender, args) => { Console.WriteLine("Hello World"); };
  test1 = null;
  GC.Collect();
  GC.WaitForPendingFinalizers();

  var test2 = new Source();
  test2.Event += test2.Handler;
  test2 = null;
  GC.Collect();
  GC.WaitForPendingFinalizers();
}

public class Source()
{
  public event EventHandler Event;

  ~Source() { Console.WriteLine("disposed"); }

  public void Handler(object sender, EventArgs args) { }
}

You will see that "disposed" is printed twice to the console verifying that both instances were collected without unregistering the event. The reason the object referenced by test2 gets collected is because it remains an isolated entity in the reference graph (once test2 is set to null that is) even though it has a reference back to itself though the event.

Now, where things get tricky is when you want to have the event target have a lifetime that is shorter than the event source. In that case you have to unregister the events. Consider the following code that demonstrates this.

public static void Main()
{
  var parent = new Parent();
  parent.CreateChild();
  parent.DestroyChild();
  GC.Collect();
  GC.WaitForPendingFinalizers();
}

public class Child
{
  public Child(Parent parent)
  {
    parent.Event += this.Handler;
  }

  private void Handler(object sender, EventArgs args) { }

  ~Child() { Console.WriteLine("disposed"); }
}

public class Parent
{
  public event EventHandler Event;

  private Child m_Child;

  public void CreateChild()
  {
    m_Child = new Child(this);
  }

  public void DestroyChild()
  {
    m_Child = null;
  }
}

You will see that "disposed" is never printed to the console demonstrating a possible memory leak. This is a particularly difficult problem to deal with. Implementing IDisposable in Child will not solve the problem because there is no guarentee that callers will play nicely and actually call Dispose.

The Answer

If your event source implements IDisposable then you have not really bought yourself anything new. That is because if the event source is no longer rooted than the event target will no longer be rooted as well.

If your event target implements IDisposable then it could clear itself from the event source but there is no guarentee that Dispose will get called.

I am not saying that unregistering events from Dispose is wrong. My point is that you really need to examine how your class hierarchy is defined and consider how you might best avoid the memory leak problem if one even exists.