views:

43

answers:

2

It's hard to find at design/compile time types that are IDisposable but that are not disposed correctly. What methods are there at runtime to find them?

+2  A: 

It is good practice to implement IDisposable if your type is resource hungry, but, this is just good practice and cannot be enforced by the compiler.

One thing you can do to make abusing IDisposables more noticeable, is to make them throw or assert in the finalizer (remember, if your type is disposed properly, the finalizer won't get called because you'll have called GC.SuppressFinalize in your dispose method). The following program shows an error in the debugger's output window when the app finishes because Hog is not correctly disposed.

    class Program
    {
        static void Main(string[] args)
        {
            new Hog( ) ;
        }
    }

    class Hog : IDisposable
    {          
        public void Dispose( )
        {
            Dispose( true ) ;
            GC.SuppressFinalize( this ) ;
        }

        protected virtual void Dispose( bool disposing )
        {
            GC.SuppressFinalize( this );
        }

        ~Hog( )
        {
            Debug.Fail( @"You didn't dispose me!" ) ;
            Dispose( false ) ;
        }
    }

You'll see the following error in the debugger:

---- DEBUG ASSERTION FAILED ----
---- Assert Short Message ----
You didn't dispose me!
---- Assert Long Message ----
    at Hog.Finalize()

but, if you did use the disposable object correctly, such as:

static void Main(string[] args)
{
    using (new Hog())
        ;
}

...you won't see anything.

To make things even more useful, you can record the current stack trace in the constructor and dump it out in the destructor. So the new, more useful Hog would look like:

    class Hog : IDisposable
    {
        readonly StackTrace _stackTrace ;

        public Hog( )
        {
#if DEBUG
            _stackTrace = new StackTrace();
#endif
        }

        public void Dispose( )
        {
            Dispose( true ) ;
            GC.SuppressFinalize( this ) ;
        }

        protected virtual void Dispose( bool disposing )
        {
            GC.SuppressFinalize( this );
        }

        ~Hog( )
        {
#if DEBUG
            Debug.WriteLine("FinalizableObject was not disposed" + _stackTrace.ToString());
#endif
            Dispose( false ) ;
        }
    }

and using it (without disposing it) would give you this in the debugger output window:

FinalizableObject was not disposed   at ConsoleApplication1.Hog..ctor()
   at ConsoleApplication1.Program.Main(String[] args)
   at System.AppDomain._nExecuteAssembly(Assembly assembly, String[] args)
   at System.AppDomain.ExecuteAssembly(String assemblyFile, Evidence assemblySecurity, String[] args)
   at Microsoft.VisualStudio.HostingProcess.HostProc.RunUsersAssembly()
   at System.Threading.ThreadHelper.ThreadStart_Context(Object state)
   at System.Threading.ExecutionContext.Run(ExecutionContext executionContext, ContextCallback callback, Object state)
   at System.Threading.ThreadHelper.ThreadStart()
Steve Dunn
You shouldn't access other managed objects in a finalizer - only release unmanaged resources. In your last example, your finalizer ~Hog is accessing a managed object _stackTrace which may already have been collected by the time the finalizer runs. As you only do this in DEBUG mode you might get away with it as long as you don't deploy debug builds in production.
Joe
How would it have been collected, apart from the Dispose method? We know Dispose hasn't been called because if it had, GC.SupressFinalize would have been called which means we wouldn't be in the finalizer.
Steve Dunn
Unfortunately this is only useful in theory. The vast majority of classes that should implement IDisposable don't need a finalizer. Make that 99.99%
Hans Passant
@Hans - best practice says you should still provide one and a good side-effect of providing one allows the checking of correct disposal.
Steve Dunn
Giving a class a destructor when it doesn't actually have some unmanaged resource to release is about as bad a practice as you can imagine. Finalizers are not cheap.
Hans Passant
@Hans - that is the whole point of the check for correctly disposing the instance. If it's correctly disposed, the finalizer isn't called. The 'best practice' that I mentioned states that you call GC.SuppressFinalize. I don't understand where the expense is coming from if it's never run.
Steve Dunn
A: 

Code analysis tools like FxCop (included in Team Editions of Visual Studio or as a free download) can detect this.

Though there will be occasional false negatives/positives.

Joe
Can you elaborate Joe? I'm aware of the R# analysis for 'put into Using construct' but that's only for locals.
Steve Dunn