views:

43

answers:

3

If I have a class with a couple of event handlers inside of it which are attached to an object that is defined within the class, am I right in thinking that I don't need to implement IDisposable to remove said handlers, even if I explicitly added the handlers myself?

Also, can anyone point me in the direction of an article that explains the scope of when it is needed to remove handlers to avoid memory leaks. (I've tried searching many times but I must be screwing up my search terms.)

ETA: Here's a situation where I think I need to remove the handlers. Please correct me if I'm wrong.

I have a collection; every time an object is added to that collection, I add a handler to a change event of the object. When I'm done with the collection, do I need to go through each item and remove the handler before setting the reference to null?

+1  A: 

It really depends -

Typically, you only have to worry about removing event handlers if the subscriber is going to outlive your object (in which case, failing to remove will prevent your object from being garbage collected), or if there is a chance that your event will be raised, and you don't want the subscribers to continue processing.

If the object subscribing to the event is internal to your class, you may not need to unsubscribe/remove the handler. In this case, when your object is unrooted, both objects will get collected (since they'll both become unrooted, provided no other references to them exist).

Reed Copsey
That's what I thought. I've just added a bit to my question which seems to be in line with the rest of what you said. Could you double check it for me please.
Jules
Actually, may be not. The subscriber in my case would be the collection, and it wouldn't outlive the object because I would have set its only reference to null.
Jules
@Jules: As long as nothing else holds references to the objects inside your collection (ie: they're ONLY in the collection), you don't even need to set them to null - as soon as the collection is unrooted, everything will (eventually) get GC'ed.
Reed Copsey
+1  A: 

IDisposable has nothing to do with events, it was designed to release unmanaged resources. You would normally only have a problem with an event handler preventing an object from getting garbage collected if the event source out-lives the event consumer. Sounds like, in your case, they'll both get collected at the same time, no need to unregister the event.

However, if the event source always out-lives the consumer then you have a problem. You'll need some kind of trigger to unregister the event handler. If the consumer already implements Dispose() then that could be the place to unregister the handler. If not then you should really consider using a plain callback method instead of an event.

Hans Passant
"if the event source out-lives the event consumer". Is this line correct or did you mean it the other way round? It seems to be the opposite of Reed's line "if the subscriber is going to outlive your object"
Jules
No, that's correct. A subscriber cannot outlive the source, the registered event handler will keep the source from getting garbage collected. The handler will just stop getting called.
Hans Passant
+1  A: 

You would want to remove the handlers if the containing object is being kept in limbo after all references to it in your application have been removed.

For example...

public class LostInLimbo
{
  private Villain _problem;

  public void SetVillain(Villain problem)
  {
    problem.SomeEvent += this.SomeHandler;
    _problem = problem;
  }
}

Given this class, if we do the following:

Villain foo = new Villain();
LostInLimbo victim = new LostInLimbo();
victim.SetVillain(foo);
victim = null;

the instance victim is now "leaked". It is referenced by foo and therefore will not be collected; however, you cannot access this instance. Do this a few hundred thousand times and you might have an issue.

In this case, you would want LostInLimbo to implement IDisposable, where you could unhook the event:

var temp = _problem;
_problem = null;
if (temp != null_
  temp -= this.SomeHandler;

Similarly, you might end up with a leak if you do this:

public class LostInLimbo
{
  public Villain Problem {get;private set;}

  public LostInLimbo()
  {
    Problem = new Villain();
    Problem.SomeEvent += this.SomeHandler;
  }
}

and do the following

var temp = new LostInLimbo().Villain;

Same situation. Villain holds a reference to the instance of LostInLimbo, but you do not have access to this instance. Again, implementing IDisposable will allow the instance of LostInLimbo to unhook itself. Of course, there are other issues with this, but its just an example.

If, however, you have this situation:

public class LostInLimbo
{
  private Villain _problem;
  public LostInLimbo()
  {
    _problem = new Villain();
    _problem.SomeEvent += this.SomeHandler;
  }
}

there are no worries. Only LostInLimbo holds a reference to _problem, and _problem holds no references to any instance of LostInLimbo except for the one that "owns" it. Once that instance is collected, so is _problem.


In response to your update, I would do the following.

First, when an item is added to the collection, the collection hooks to the item (override any method that adds an item to the collection). When an item is removed from the collection, the collection unhooks from the item (again, override any method that removes an item from the collection). I would also implement IDisposable to empty the collection on dispose. I would make sure that any type that used the collection also implements IDisposable.

Will
As you've explained it is exactly what I do at the moment, However, from reading your answer, it doesn't seem that I need to empty the collection when I'm done with it, as long as I set the reference to the collection to null. Is that just for completeness?
Jules
@Jules I usually do that, for completeness. If I'm disposing of a collection I'll get all items in the collection, clear out the collection, then foreach through what I've got and dispose of them. Just my particular habit, and it makes it easier to have a Dispose method that you can call more than once without throwing an exception.
Will