33
\$\begingroup\$

I commonly run into the need to implement IDisposable in my code. To correctly dispose of both managed and unmanaged resources requires a reasonable amount of boilerplate code. Based on the documentation found here I have created the following base class.

My intent is that any object I create that needs to dispose of resources doesn't need to rewrite the boilerplate code. Rather it can inherit from this object and implement two abstract methods.

Does this appear correct? And has anyone else written something similar? I would appreciate comments on the style, correctness and how well this conforms to best practices.

public abstract class DisposableObject : IDisposable
{
    private bool _disposed = false;

    public bool Disposed
    {
        get { return _disposed; }
    }

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

    ~DisposableObject()
    {
        Dispose(false);
    }

    private void Dispose(bool disposing)
    {
        if (!_disposed)
        {
            if (disposing)
            {
                DisposeManagedResources();
            }

            DisposeUnmanagedResources();

            _disposed = true;
        }
    }

    protected abstract void DisposeManagedResources();
    protected abstract void DisposeUnmanagedResources();
}

EDIT: Final Implementation

public abstract class DisposableObject : IDisposable
{
    public bool Disposed { get; private set;}      

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

    ~DisposableObject()
    {
        Debug.Assert(Disposed, "WARNING: Object finalized without being disposed!");
        Dispose(false);
    }

    private void Dispose(bool disposing)
    {
        if (!Disposed)
        {
            if (disposing)
            {
                DisposeManagedResources();
            }

            DisposeUnmanagedResources();
            Disposed = true;
        }
    }

    protected virtual void DisposeManagedResources() { }
    protected virtual void DisposeUnmanagedResources() { }
}
\$\endgroup\$
  • 3
    \$\begingroup\$ One problem I see is you'll quickly run into multiple inheritance issues (or rather the lack of). I've often seen similar implementations however, and in some scenarios where you control the entire inheritance hierarchy I guess it is useful. \$\endgroup\$ – Steven Jeuris May 31 '11 at 12:22
  • \$\begingroup\$ Inheritance (or the lack of it) is something I considered as well. Primarily I am using this class to help wrap unmanaged libraries that I want to use in managed code. For this scenario I decided I could live with the limitation on inheritance. \$\endgroup\$ – Nathanael May 31 '11 at 16:04
14
\$\begingroup\$

I would think on making two changes:

  1. Make one or both Dispose*Resources methods virtual instead of abstract. Though it highly depends on how often do you need to handle unmanaged resources. I can't remember last time I was handling them and I would hate overriding this method in each class just to make it empty.
  2. I would add some logging either to your finalizer method or to Dispose(bool disposing) method in order to catch situations when disposable object wasn't disposed correctly by calling Dispose() method. Most developers are looking for such information and you have a good place to inject it.
\$\endgroup\$
  • \$\begingroup\$ Both good suggestions, thanks! As far as logging goes, I'm considering adding assert statements to warn if Dispose is not called by the developer. \$\endgroup\$ – Nathanael May 31 '11 at 16:06
10
\$\begingroup\$

Something I've found useful in the past when implementing this pattern is to capture the stack trace during construction, and include this in the Debug.Assert message.

Knowing where the object was constructed can often be a help in tracking down the source of the issue, especially if the objects are created in one place and then need to be disposed of by some other object which takes over responsibility for their lifetime.

\$\endgroup\$
6
\$\begingroup\$

I'm in agreement with Brian, with the exception that I believe the Dispose(bool disposing) method should be abstract. My reasoning behind this, is that you must have a reason for inheriting from the DisposableObject type in the first place, in which case, having the method as virtual seems a little pointless, as it would be an optional implementation. If it's optional, why inherit from DisposableObject anyway?

Having it as virtual would be beneficial where you have some common code, but perhaps a better design would be:

protected virtual void Dispose(bool disposing)
{
  if (!Disposed)
  {
    DisposeInternal(disposing);
    Disposed = true;
  }
}

protected abstract void DisposeInternal(bool disposing);

That way you can assure that DisposeInternal is only ever called when you are actually disposing, and it doesn't need to set the Dispose flag, as this is handled by the outer call.

\$\endgroup\$
  • \$\begingroup\$ My comment about making the method virtual as opposed to abstract was specifically regarding the case where there was a separate method to clean-up managed and unmanaged resources. In the case where there is a single clean-up method, I would agree that it should be abstract. \$\endgroup\$ – Brian Reichle Jun 3 '11 at 3:29
  • \$\begingroup\$ In your sample code, you should call DisposeInternal() regardless of disposing and pass disposing into it, since unmanaged resources should be cleaned up on finalising if not already disposed of properly. \$\endgroup\$ – Brian Reichle Jun 3 '11 at 3:38
  • \$\begingroup\$ @Brian - Good point, I'll update my answer. \$\endgroup\$ – Matthew Abbott Jun 3 '11 at 6:10
  • 1
    \$\begingroup\$ I disagree that this function should be public. There is never a situation where it is correct for the user of the class to call Dispose(false). This distinction is only provided so that at runtime, if the developer has forgotten to call Dispose on the object, we won't be competing with the GC to dispose of any remaining objects. \$\endgroup\$ – Nathanael Jun 3 '11 at 19:01
  • \$\begingroup\$ Another good point, I've changed it to protected... \$\endgroup\$ – Matthew Abbott Jun 3 '11 at 22:51
3
\$\begingroup\$

Personally I would have made Dispose(bool disposing) protected virtual rather than have 2 abstract methods ... for consistency with most unsealed disposable objects in the framework. Also, I recall FxCop was quite pedantic about the implementation of IDisposable.

If you still want to use use the two methods, I would suggest making at-least DisposeUnmanagedResources() virtual rather than abstract as a convenience to inheritors. I find it much more common for my disposable objects to dispose other objects rather than clean-up unmanaged resources of their own.

\$\endgroup\$
3
\$\begingroup\$

If this is restricted to wrapping unmanaged libraries, then it could be useful, but I wouldn't ever use it where there are no unmanged resources, it'll completely screw-up the garbage-collector- as one should not have a finalizer unless one absolutely needs it.

You must implement finalizers very carefully; it's a complex operation that can carry considerable performance overhead.

\$\endgroup\$
  • \$\begingroup\$ I would go so far as to suggest a separate subclass of this DisposableObject class implementing the finalizer. Most subclasses would inherit directly from DisposableObject, but if you need to write a quick wrapper for something unmanaged then you could use the specialized abstract subclass with a finalizer. \$\endgroup\$ – binki Jul 3 '14 at 19:11
3
\$\begingroup\$

Adding a method for checking and throwing can clean up a lot of noise.

protected void VerifyDisposed()
{
    if (_disposed)
    {
        throw new ObjectDisposedException(GetType().FullName);
    }
}
\$\endgroup\$

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service, privacy policy and cookie policy

Not the answer you're looking for? Browse other questions tagged or ask your own question.