The project I've been working on has finally released its first version to the internal customers; the push to release has been the major cause for my silence. As part of the development process we've been performing code reviews for the permanent members of staff to see how our mentoring has taken. Yesterday I came across an interesting (to me anyway) section of code, an implementation of IDispose. Setting aside the fact that the class had no unmanaged resources to discard and the method was being used to save an XmlDocument that had been created on object construction, something that should be in a publicly exposed method, the code pattern used for the implementation made me pause.

The implementation followed a finalize and dispose pattern from MSDN. A nice simple piece of code; until you start thinking about multi-threading. Now I'll admit that the idea of creating multiple threads that act upon, and could call .Dispose() upon the same object is a pretty silly idea but it provided a good example of how to code defensively. The problem arises because the object has no concept of its internal disposed state and two calls to .Dispose() from different threads will cause the MSDN pattern to attempt to deallocate unmanaged resources twice.

A better solution would be to add a private field to your object to track state and lock your object before performing a check on state and any disposal necessary. In addition we can now check if the object is mid-disposal and throw the appropriate exception if a method or property is called. By separating the code that performs the release actions you need to perform from the actual Dispose method you end up with a cleaner inheritance model, where subclasses override your method, but leave the implementation of the Dispose method untouched.

public class BaseClass : IDisposable
{
  private bool isDisposed = false;
  private Object syncBlock = new Object();

  // This is no longer virtual, subclasses
  // should not override this method
  public void Dispose()
  {
    lock(syncBlock)
    {
      if (!isDisposed)
      {
        performDispose();
        isDisposed = true;
        GC.SuppressFinalize(this);
      }
    }
  }

  protected virtual void performDispose()
  {
    // Release any unmanaged resources
  }

  ~BaseClass()
  {
    // Destructors only get called if Dispose()
    // has not be called.
    performDispose();
  }

  protected bool Disposed
  {
    get
    {
      lock(syncBlock)
      {
        return isDisposed;
      }
    }
  }

  public void SampleMethod()
  {
    // Wrap every method with this check
    if (Disposed)
    {
      throw new
        ObjectDisposedException("Object is already disposed");
    }
  }
}

Subclasses simply need to override performDispose(), remembering to call base.performDispose() and finally you have thread safe Dispose methods.

[16:21] Embarassing enough, Phil of the unbelievable surname pointed out that by removing one race condition I had produced another by locking on this, a common place, lazy, locking solution but one not recommended. Then again, if you've locked an object to dispose it, please, justify why you're doing that <g>.

As he also points out in the email conversation following his comment, locking on a value type is a bad idea, you only lock on objects. If I attempted to lock on isDisposed you'll hopefully see a compiler error, but at worst it gets boxed, and the lock is performed on the new instance, so you will never get the effect you wanted. I really need to get away from 3 months of writing SQL code and get back into the C# mindset.