c# - 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

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.