Is this IDisposable implementation correct?

Question

I can never remember all the rules for implementing the IDisposable interface, so I tried to come up with a base class that takes care of all of this and makes IDisposable easy to implement. I just wanted to hear your opinion if this implementation is ok as it is or whether you see something I could improve. The user of this base class is supposed to derive from it and then implement the two abstract methods ReleaseManagedResources() and ReleaseUnmanagedResources(). So here is the code:

public abstract class Disposable : IDisposable
{
    private bool _isDisposed;
    private readonly object _disposeLock = new object();

    /// <summary>
    /// called by users of this class to free managed and unmanaged resources
    /// </summary>
    public void Dispose() {
        DisposeManagedAndUnmanagedResources();
    }

    /// <summary>
    /// finalizer is called by garbage collector to free unmanaged resources
    /// </summary>
    ~Disposable() { //finalizer of derived class will automatically call it's base finalizer
        DisposeUnmanagedResources();
    }

    private void DisposeManagedAndUnmanagedResources() {
        lock (_disposeLock) //make thread-safe
            if (!_isDisposed) { //make sure only called once
                try { //suppress exceptions
                    ReleaseManagedResources();
                    ReleaseUnmanagedResources();
                }
                finally {
                    GC.SuppressFinalize(this); //remove from finalization queue since cleaup already done, so it's not necessary the garbage collector to call Finalize() anymore
                    _isDisposed = true;
                }
            }
    }

    private void DisposeUnmanagedResources() {
        lock (_disposeLock) //make thread-safe since at least the finalizer runs in a different thread
            if (!_isDisposed) { //make sure only called once
                try { //suppress exceptions
                    ReleaseUnmanagedResources();
                }
                finally {
                    _isDisposed = true;
                }
            }
    }

    protected abstract void ReleaseManagedResources();
    protected abstract void ReleaseUnmanagedResources();

}

Answer 1

EDIT Yes, I misread the portion about separating disposing managed and unmanaged resources (still on first cup of coffee).

However the lock is still almost certainly not necessary. Yes, during a passive dispose the code will be running on the finalizer thread which is different than the thread it originally ran on. However if the object is being finalized in this fashion the CLR has already determined there are no existing references to this object and hence collected it. So there is no other place which can be calling your dispose at that time and hence there is no reason for a lock.

A couple of other style comments.

Why make the methods abstract? By doing so you are forcing derived classes to implement methods to dispose of managed and unmanaged resources even if they don't have any to dispose. True there is no point in deriving from this class if you don't have anything to dispose. But I think it's fairly common to only have one or the other but not both. I would make them virtual vs. abstract.

Also you prevent double dispose but you do nothing to warning the developer that they are double disposing an object. I realize the MSDN documentation says that double dispose should be essentially a no-op but at the same time under what circumstances should this be legal? In general it's a bad idea to access an object after it's been disposed. A double dispose necessitates a disposed object being reused and is likely a bug (this can happen if finalization is not suppressed in an active dispose but that's also a bad idea). If I were implementing this I would throw on double dispose to warn the developer they were using an object incorrectly (that is using it after it was already disposed).

Answer 2

I can't really comment on the correctness of your code, but I question whether you'll find having the Disposable class at the base of your inheritance tree as flexible as it needs to be. It's won't be much use when you want to inherit from something you don't own. I think that IDispoasble is left as an interface because of the many different domains across which it might be used. Providing a concrete implementation at the foot of the inheritance hierarchy would be... limiting.

Answer 3

You are accessing the managed object _disposeLock in the finalizer. It may have already been garbage-collected by then. Not sure what the implications of this would be as you are only using it to lock.

Thread-safety seems like overkill. I don't think you need to worry about contention between the GC thread and your application thread.

Otherwise it looks correct.

Answer 4

If you are concerned about the thread safety then I'd use the lightweight interlocked operations rather than a heavyweight lock:

class MyClass: IDispose {
  int disposed = 0;

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

  public virtual void Dispose(bool disposing)
  {
     if (0 == Thread.InterlockedCompareExchange(ref disposed, 1, 0))
     {
       if (disposing)
       {
          // release managed resources here
       }
       // release unmanaged resources here
     }
  }

  ~MyClass()
  {
    Dispose(false);
  }
}