A Better Implementation Pattern for IDisposable

by casperOne 4. January 2009 19:18

During the first few years of programming in .NET, I sheepishly followed the Design Guidelines for Class Library Developers in regards to implementing IDisposable in my class libraries.

On the face of things, the recommendation is solid. A base class exposing IDisposable looks like this:

// Design pattern for a base class.
public class Base : IDisposable
{
    //Implement IDisposable.
    public void Dispose()
    {
        Dispose(true);
        GC.SuppressFinalize(this);
    }

    protected virtual void Dispose(bool disposing)
    {
        if (disposing)
        {
            // Free other state (managed objects).
        }
        // Free your own state (unmanaged objects).
        // Set large fields to null.
    }

    // Use C# destructor syntax for finalization code.
    ~Base()
    {
        // Simply call Dispose(false).
        Dispose(false);
    }
}

So what’s going on here?

First, a non-virtual method is exposed on the class so derived classes don’t have the opportunity to muck up cleanup code.

Next, a protected virtual override of Dispose handles the disposal of unmanaged resources/large objects as well as any other state that needs to be managed (if not called from the finalizer, then any references to other IDisposable implementations have their Dispose methods called as well).  This allows for base classes to be able to hook into the cleanup code without burdening it with the details of implementing the finalizer or the method on IDisposable:

// Design pattern for a derived class. 
public class Derived : Base
{
    protected override void Dispose(bool disposing)
    {
        if (disposing)
        {
            // Release managed resources. 
        }
        // Release unmanaged resources. 
        // Set large fields to null. 
        // Call Dispose on your base class. 
        base.Dispose(disposing);
    }
    // The derived class does not have a Finalize method 
    // or a Dispose method with parameters because it inherits 
    // them from the base class. 
}

(Both code samples were copied from “Implementing Finalize and Dispose to Clean Up Unmanaged Resources”)

Last, the finalizer calls the same protected virtual overload of Dispose, indicating that it should only clean up unmanaged resources and large objects (not references that implement IDisposable).

The effect here is twofold.  The first is that calling Dispose has the effect of freeing any state that needs freeing at a determined point in time (usually when you are done with the resource).

The second is that if you forget to call Dispose, the garbage collector will do it for you.

The problem lies with the statement “if you forget to call Dispose.”

Pop quiz, how many of the following errors in code would be tolerated in a production environment:

  • Not hooking up an event handler for an event that code needs to react to
  • Not calling Monitor.Exit (unlikely, but a possibility if not using the lock statement or calling TryEnter) after a call to Monitor.Enter/Monitor.TryEnter
  • Not throwing an exception when an error occurs (one that is worthy of an exception, that is)
  • Not synchronizing access to a resource that is accessed by multiple threads of execution

In any of these cases, this would be considered a bug and fixed.

But why is this a bug?

If the finalizer executes (remember, the object is removed from the finalization queue if Dispose is called) then that means that the instance did not have the Dispose method called on it.  In this case, you have circumvented the entire reason for implementing IDisposable in the first place, which is to make the developer aware that attention needs to be paid to instances of the implementing type.

Microsoft exacerbates the situation with the current guidelines because it actually obfuscates the issue.  With the recommendation, if Dispose is never called then the resource is eventually freed up at some indeterminate point of time in the future.

That recommendation ends up placing this on the same level as the race-condition bug noted in the last bullet point .  Your app might run very smoothly for the most part, but at random times, you have exceptions that you can’t trace (because the resource in question is corrupted and leads to states in your application that you aren’t prepared for most likely).

In the case of not calling Dispose, this will most likely manifest itself in the form of starvation of the resource managed by the IDisposable implementation at random times (sockets, database connections, etc., etc.) and even then, if at all, depending on the usage pattern of your app.  You might never see the exception, but then (at a critical moment, of course) your app fails, and you haven’t the faintest reason why.

So what can I do to prevent this?

The bad news is that outside of your code, not much.  You have to live with the fact that everything in the framework and most other library code written follows this guideline.  If you miss cleaning up one of these resources and you run into an intermittent problem involving a resource that isn’t cleaned up properly, a great deal of painful debugging awaits you.

To add to that, I haven’t seen many static-analysis tools that catch this either.

Quite simply, there isn’t a definite way to catch this at compile-time (or to be more specific, without running code).

What about inside my code?

That is a different story.  You can simply throw an exception in the finalizer.

You are surely thinking now “woah, that’s a massively bad idea”.  Well, yes, it is.  You are going to hose your app in a major way either during the garbage collection that occurs with the IDisposable implementation which hasn’t had Dispose called on it, or on exit of the app.

But that’s the point.

Surely you aren’t going to deliver your app without running it at least once, right?  And if you are in a more formalized software development environment, you are going to run it through all sorts of unit tests, quality assurance tests, and so on.

So even if you can’t catch the error at compile time, you can catch it much earlier, and hopefully, before your app ships (assuming you have proper test coverage of your app).

Here is the code sample.  For all intents and purposes, the derived class remains the same:

// Design pattern for a derived class.
public class Derived : Base
{
    protected override void Dispose(bool disposing)
    {
        if (disposing)
        {
            // Release managed resources.
        }
        // Release unmanaged resources.
        // Set large fields to null.
        // Call Dispose on your base class.
        base.Dispose(disposing);
    }
    // The derived class does not have a Finalize method
    // or a Dispose method with parameters because it inherits
    // them from the base class.
}

The changes come in the base class:

// A better design pattern for a base class.
public class Base : IDisposable
{
    // Implement IDisposable.
    // This remains the same, this instance should not be finalized if Dispose is called.
    public void Dispose()
    {
        Dispose(true);
        GC.SuppressFinalize(this);
    }

    protected virtual void Dispose(bool disposing)
    {
        if (disposing)
        {
            // Free other state (managed objects).
        }
        // Free your own state (unmanaged objects).
        // Set large fields to null.
    }

    // The change is here, you want to throw your exception if you do not call Dispose.
    ~Base()
    {
        // Throw an exception.
throw new InvalidOperationException("Dispose method not called."); } }

Things to note:

  • The public overload of Dispose is the same.
  • The protected overload has not been changed, but only to make it easier for those that already have Dispose implementations following the original guideline.  Ideally, it would be changed to a method with no parameter (since only true will ever be passed here, there is no point in having it, as well as the conditional with the check against the parameter) and renamed (since it would conflict with the public Dispose method).  Do NOT refactor the public method into a virtual method, as that would give derived classes the ability to screw things up.
  • The exception is thrown in the finalizer.  I’d recommend resource strings for localization purposes of course.

Tags: , ,

programming