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

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

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.

Answer 2

Just implementing Dispose() across all types is not going to solve your problem. Remember that Dispose() is not automatically called and it has nothing to do with reclaiming managed memory. In order to have any effect of your Dispose() methods, you need to call it in all relevant place - either explicitly or via using.

In other words just implementing IDisposable all around will not magically solve your problems cause the Dispose() methods will not be called unless you also change the usage of every type in your code.

However, I would not recommend implementing IDisposable on all your types, simply because it makes no sense. The interface is used to indicate that the type in question uses some resource, which isn't handled by the garbage collector.

Event references are handled by the garbage collector. You just need to unsubscribe if your publisher lives significantly longer than your subscribers. Once the publisher dies the subscribers will die as well.

Answer 3

I once helped a coworker out on a similar problem with OutOfMemoryException errors occuring (caused by events leaving object references hanging around). The first thing I did was run the code through FXCop, which highlighted Dispose wasn't called on an IDisposable class.

Modifying that code to be disposed fixed the issue. Perhaps you should recommend FXCop be used?

Perhaps find the code with the issue in the source repository, run FXCop on it- see if it highlights the issue (it probably will if it is caused by a .NET Framework class) and use that to convince your coworkers.

Answer 4

IDisposable is only meant to be used to release unmanaged resources (SafeHandles and the like), and the Dispose method propagates through the class heirarchy. It is not meant to try and get around bad unmanaged programming practices.

Answer 5

Create duty on resolving one more OutOfMemoryException. When somebody resolves errors belong to others, it raises responsibility on own code. At early stage of our project we have been recommended "flag of fool" - responsible on fatal error gets this flag for a day.

Answer 6

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 7

It's a hard one to pull off, but I've found the best way to get people to do something you want is to let them think it's their idea. You know them better than me, but phrases like "If only there was a way to find out why the objects are hanging around so long" and "I wish I knew more about events holding on to objects" might be a starting point.

Answer 8

Ask them if they would like to be forced to turn off the motor after using a bicycle, even if there isn't a motor.

Or if they would like to be forced to press a "turn off" button on their chair, desk, coffee cup and other stuff before their leave their workplace, even if there is nothing to be turned off.

Implementing IDisposable forces the user to explicitly tell the object when it is not used anymore. If this object does not need to clean up anything, it is just a unnecessary complexity.

By the way, IMHO unregistering events by implementing IDisposable is a suitable way to clean up events. There is something to "turn off".

Answer 9

Propose a solution that is much superior than their solution ;-)

For example, a simple alternative could be to use WeakReferences in the long-living objects to hold the references to the event handlers. This would require that the event handlers are referenced somewhere else as long as they're needed, but as soon as they'd go out of scope, they're garbage collected and the weak references can be removed from the long-living objects.

Answer 10

In general, if you register a handler with an event, then "unregistering" is just the sort of basic cleanup every object should do upon being destroyed. If you don't have destructors in the language, then you have to define a method to call to tell the object it's going away. That method should clean up the event handlers.

Isn't this what IDisposable is for? Why would you need another solution if it's already there? And why didn't those knuckleheads implement their objects correctly in the first place? ;)