How do I convince my colleagues not to implement IDisposable on everything? [closed]

Question

I work on a project where there is a huge number of objects being instanced by a few classes that stay in memory for the lifetime of the application. There are a lot of memory leaks being caused with OutOfMemoryExceptions being thrown every now and again. It seems like after the instantiated objects ago out of scope, they are not being garbage collected.

I have isolated the problem to being mostly about the event handlers that are attached to the long-living object that are never detached, thus causing the long-living object to still have a reference to the out of scope objects, which then will never be garbage collected.

The solution that has been proposed by my colleagues is as follows: Implement IDisposable on all classes, across the board and in the Dispose method, null all the references in your objects and detach from all event that you attached to.

I believe this is a really really bad idea. Firstly because it's 'overkill' since the problem can be mostly solved by fixing a few problem areas and secondly because the purpose of IDisposable is to release any unmanaged resources your objects control, not because you don't trust the garbage collector. So far my arguments have fallen on deaf ears. How can I convince them that this is futile?

Answer 1

By coincidence I just posted this comment elsewhere:

An reference to an object being incorrectly retained is still a resource leak. This is why GC programs can still have leaks, usually due to the Observer pattern - the observer is on a list instead the observable and never gets taken off it. Ultimately, a remove is needed for every add, just as a delete is needed for every new. Exactly the same programming error, causing exactly the same problem. A "resource" is a really just a pair of functions that have to be called an equal number of times with corresponding arguments, and a "resource leak" is what happens when you fail to do that.

And you say:

the purpose of IDisposable is to release any Unmanaged resources your objects controls

Now, the += and -= operators on an event are effectively a pair of functions that you have to call an equal number of times with corresponding arguments (the event/handler pair being the corresponding arguments).

Therefore they constitute a resource. And as they are not dealt with (or "managed") by the GC for you, it can be helpful to think of them as just another kind of unmanaged resource. As Jon Skeet points out in the comments, unmanaged usually has a specific meaning, but in the context of IDisposable I think it's helpful to broaden it to include anything resource-like that has to be "torn down" after it has been "built up".

So event detaching is a very good candidate for handling with IDisposable.

Of course, you need to call Dispose somewhere, and you don't need to implement it on every single object (just those with event relationships that need management).

Also, bear in mind that if a pair of objects are connected by an event, and you "cast them adrift", by losing all references to them in all other objects, they don't keep each other alive. GC doesn't use reference counting. Once an object (or island of objects) is unreachable, it is up for being collected.

You only have to worry about objects enlisted as event handlers with events on objects that live a long time. e.g. a static event such as AppDomain.UnhandledException, or events on your application's main window.

Answer 2

Point them at Joe Duffy's post about IDisposable/finalizers - combined wisdom of many smart people.

I'm currently finding it hard to see a statement there saying "don't implement it when you don't need it" - but aside from anything else, showing them the complexity involved in implementing it properly may well help to dissuade them from it...

Unfortunately, if people won't listen, they won't listen. Try to get them to explain why they think they need IDisposable. Do they think the garbage collector doesn't work? Show them that it works. If you can convince them that it's doing no good (for most types) then surely they'll stop adding work for themselves...

As Brian says, implementing IDisposable isn't going to help with the event problem on its own - it needs to actually be called by something. Finalizers aren't going to help you in this case either. They really need to explicitly do something to remove the event handlers.