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.

Answer 2

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 3

The event-handling pattern that's automatically generated using the vb.net WithEvents keyword is a pretty decent one. The VB code (roughly):

WithEvents myPort As SerialPort

Sub GotData(Sender As Object, e as DataReceivedEventArgs) Handles myPort.DataReceived
Sub SawPinChange(Sender As Object, e as PinChangedEventArgs) Handles myPort.PinChanged

will get translated into the equivalent of:

SerialPort _myPort;
SerialPort myPort
{  get { return _myPort; }
   set {
      if (_myPort != null)
      {
        _myPort.DataReceived -= GotData;
        _myPort.PinChanged -= SawPinChange;
      }
      _myPort = value;
      if (_myPort != null)
      {
        _myPort.DataReceived += GotData;
        _myPort.PinChanged += SawPinChange;
      }
   }
}

This is a reasonable pattern to follow; if you use this pattern, then in Dispose you would set to null all properties that have associated events, which will in turn take care of unsubscribing them.

If one wanted to automate disposal slightly, so as to ensure that things got disposed, one could change the property to look like this:

Action<myType> myCleanups; // Just once for the whole class
SerialPort _myPort;
static void cancel_myPort(myType x) {x.myPort = null;}
SerialPort myPort
{  get { return _myPort; }
   set {
      if (_myPort != null)
      {
        _myPort.DataReceived -= GotData;
        _myPort.PinChanged -= SawPinChange;
        myCleanups -= cancel_myPort;
      }
      _myPort = value;
      if (_myPort != null)
      {
        myCleanups += cancel_myPort;
        _myPort.DataReceived += GotData;
        _myPort.PinChanged += SawPinChange;
      }
   }
}
// Later on, in Dispose...
  myCleanups(this);  // Perform enqueued cleanups

Note that hooking static delegates to myCleanups means that even if there are many instances of myClass, there will only have to be one copy of each delegate system-wide. Perhaps not a big deal for classes with few instances, but potentially significant if a class will be instantiated many thousands of times.

Answer 4

My preference is to manage lifetime with a disposable. Rx includes some disposable extensions which let you do the following:

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

If you then store this in some kind of GroupDisposable thats lifetimed to the correct scope then you are all set.

If you aren't managing lifetime with disposables and scopes then definitely worth investigating as its becoming a very pervasive pattern in .net.