C# abstract Dispose method

Question

I have an abstract class that implements IDisposable, like so:

public abstract class ConnectionAccessor : IDisposable
{
    public abstract void Dispose();
}

In Visual Studio 2008 Team System, I ran Code Analysis on my project and one of the warnings that came up was the following:

Microsoft.Design : Modify 'ConnectionAccessor.Dispose()' so that it calls Dispose(true), then calls GC.SuppressFinalize on the current object instance ('this' or 'Me' in Visual Basic), and then returns.

Is it just being silly, telling me to modify the body of an abstract method, or should I do something further in any derived instance of Dispose?

Answer 1

The warning basically tells you to implement the Dispose pattern in your class.

The resulting code should look like this:

public abstract class ConnectionAccessor : IDisposable
{
    ~ConnectionAccessor()
    {
        Dispose(false);
    }

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

    protected virtual void Dispose(bool disposing)
    {
    }
}

Answer 2

You should follow the conventional pattern for implementing Dispose. Making Dispose() virtual is considered bad practice, because the conventional pattern emphasizes reuse of code in "managed cleanup" (API client calling Dispose() directly or via using) and "unmanaged cleanup" (GC calling finalizer). To remind, the pattern is this:

public class Base
{
    ~Base()
    {
        Dispose(false);
    }

    public void Dispose()
    {
        Dispose(true);
        GC.SuppressFinalize(this); // so that Dispose(false) isn't called later
    }

    protected virtual void Dispose(bool disposing)
    {
        if (disposing)
        {
             // Dispose all owned managed objects
        }

        // Release unmanaged resources
    }
}

Key here is that there's no duplication between finalizer and Dispose for unmanaged cleanup, and yet any derived class can extend both managed and unmanaged cleanup.

For your case, what you should do is this:

protected abstract void Dispose(bool disposing)

and leave everything else as is. Even that is of dubious value, since you're enforcing your derived classes to implement Dispose now - and how do you know that all of them need it? If your base class has nothing to dispose, but most derived classes likely do (with a few exceptions, perhaps), then just provide an empty implementation. It is what System.IO.Stream (itself abstract) does, so there is precedent.

Answer 3

The warning is interesting though. Eric Lippert, one of the C# designers, blogged about why error messages should be "Diagnostic but not prescriptive: describe the problem, not the solution". Read here.

Answer 4

While it does seem a little like nit-picking, the advice is valid. You are already indicating that you expect any sub-types of ConnectionAccessor will have something that they need to dispose. Therefore, it does seem better to ensure that the proper cleanup is done (in terms of the GC.SuppressFinalize call) by the base class rather than relying on each subtype to do it.

I use the dispose pattern mentioned in Bruce Wagners book Effective C# which is basically:

public class BaseClass : IDisposable
{
    private bool _disposed = false;
    ~BaseClass()
    {
        Dispose(false);
    }

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

    protected virtual void Dispose(bool disposing)
    {
        if (_disposed)
            return;

        if (disposing)
        {
            //release managed resources
        }

        //release unmanaged resources

        _disposed = true;
    }
}

public void Derived : BaseClass
{
    private bool _disposed = false;

    protected override void Dispose(bool disposing)
    {
        if (_disposed) 
            return;

        if (disposing)
        {
            //release managed resources
        }

        //release unmanaged resources

        base.Dispose(disposing);
        _disposed = true;
    }

Answer 5

The only gripe I'd have with the answers provided so far is that they all assume that you need to have a finalizer, which isn't necessarily the case. There is a fairly significant performance overhead associated with finalization, which I wouldn't want to impose on all of my derived classes if it wasn't necessary.

See this blog post by Joe Duffy, which explains when you may or may not need a finalizer, and how to properly implement the Dispose pattern in either case.
Summarizing Joe's blog post, unless you're doing something fairly low-level dealing with unmanaged memory, you shouldn't implement a finalizer. As a general rule of thumb, if your class only holds references to managed types that implement IDisposable themselves, you don't need the finalizer (but should implement IDisposable and dispose of those resources). If you are allocating unmanaged resources directly from your code (PInvoke?) and those resources must be freed, you need one. A derived class can always add a finalizer if it really needs it, but forcing all derived classes to have a finalizer by putting it in the base class causes all derived classes to be impacted by the performance hit of finalizable objects when that overhead may not be necessary.