Is this a legitimate alternative to the "traditional" dispose pattern for class hierarchies?

Question

I am not a fan of boilerplate code: copy-paste reuse is potentially error-prone. Even if you use code snippets or smart templates, there is no guarantee the other developer did, which means there's no guarantee they did it right. And, if you have to see the code, you have to understand it and/or maintain it.

What I want to know from the community is: is my implementation of IDispose for a class hierarchy a legitimate alternative to the "traditional" dispose pattern? By legitimate, I mean correct, reasonably well performing, robust, and maintainable.

I am ok with this alternative being plain wrong, but if it is, I'd like to know why.

This implementation assumes you have full control over the class hierarchy; if you don't you'll probably have to resort back to boilerplate code. The calls to Add*() would typically be made in the constructor.

public abstract class DisposableObject : IDisposable
{
  protected DisposableObject()
  {}

  protected DisposableObject(Action managedDisposer)
  {
     AddDisposers(managedDisposer, null);
  }

  protected DisposableObject(Action managedDisposer, Action unmanagedDisposer)
  {
     AddDisposers(managedDisposer, unmanagedDisposer);
  }

  public bool IsDisposed
  {
     get { return disposeIndex == -1; }
  }

  public void CheckDisposed()
  {
     if (IsDisposed)
        throw new ObjectDisposedException("This instance is disposed.");
  }

  protected void AddDisposers(Action managedDisposer, Action unmanagedDisposer)
  {
     managedDisposers.Add(managedDisposer);
     unmanagedDisposers.Add(unmanagedDisposer);
     disposeIndex++;
  }

  protected void AddManagedDisposer(Action managedDisposer)
  {
     AddDisposers(managedDisposer, null);
  }

  protected void AddUnmanagedDisposer(Action unmanagedDisposer)
  {
     AddDisposers(null, unmanagedDisposer);
  }

  public void Dispose()
  {
     if (disposeIndex != -1)
     {
        Dispose(true);
        GC.SuppressFinalize(this);
     }
  }

  ~DisposableObject()
  {
     if (disposeIndex != -1)
        Dispose(false);
  }

  private void Dispose(bool disposing)
  {
     for (; disposeIndex != -1; --disposeIndex)
     {
        if (disposing)
           if (managedDisposers[disposeIndex] != null)
              managedDisposers[disposeIndex]();
        if (unmanagedDisposers[disposeIndex] != null)
           unmanagedDisposers[disposeIndex]();
     }
  }

  private readonly IList<Action> managedDisposers = new List<Action>();
  private readonly IList<Action> unmanagedDisposers = new List<Action>();
  private int disposeIndex = -1;
}

This is a "complete" implementation in the sense I provide support for finalization (knowing most implementations don't need a finalizer), checking whether an object is disposed, etc. A real implementation may remove the finalizer, for example, or create a subclass of DisposableObject that includes the finalizer. Basically, I threw in everything I could think of just for this question.

There are probably some edge cases and esoteric situations I've missed, so I invite anyone to poke holes in this approach or to shore it up with corrections.

Other alternatives might be to use a single Queue<Disposer> disposers in DisposableObject instead of two lists; in this case, as disposers are called, they are removed from the list. There are other slight variations I can think of, but they have the same general result: no boilerplate code.

Answer 1

The first issue you will potentially hit is that C# only allows you to inherit from a single base class, which in this case will always be DisposableObject. Right here you have cluttered your class hierarchy by forcing additional layers so that classes that need to inherit from DisposableObject and some other object can do so.

You are also introducing a lot of overhead and maintenance issues down the road with this implementation (not to mention the repeated training costs everytime someone new comes on to the project and you have to explain how they should use this implementation rather than the defined pattern). You know have multiple states to keep track of with your two lists, there is no error handling around the calls to the actions, the syntax when calling an action looks "wierd" (while it may be common to invoke a method from an array, the syntax of simply putting the () after the array access just looks strange).

I understand the desire to reduce the amount of boilerplate you have to write, but disposability is generally not one of those areas that I would recommend taking short-cuts or otherwise deviating from the pattern. The closest I usually get is to use a helper method (or an extension method) that wraps the actual call to Dispose() on a given object. These calls typically look like:

if (someObject != null)
{
   someObject.Dispose();
}

This can be simplified using a helper method, but keep in mind that FxCop (or any other static analysis tool that checks for correct dispose implementations) will complain.

As far as performance is concerned, keep in mind that you are making a lot of delegate calls with this type of implementation. This is, by nature of a delegate, somewhat more costly than a normal method call.

Maintainability is definately an issue here. As I mentioned, you have the repeated training costs everytime someone new comes on to the project and you have to explain how they should use this implementation rather than the defined pattern. Not only that, you have problems with everyone remembering to add their disposable objects to your lists.

Overall, I think doing this is a bad idea that will cause a lot of problems down the road, especially as the project and team size increase.

Answer 2

I occasionally have need for tracking several open files at a time or other resources. When I do I use a utility class similar to the following. Then the object still implements the Displose() as you suggest, even tracking multiple lists (managed/unmanaged) is easy and obvious to developers whats going on. Additionally the derivation from the List object is no accident, it allows you call Remove(obj) if needed. My constructor usually looks like:

  _resources = new DisposableList<IDisposable>();
  _file = _resources.BeginUsing(File.Open(...));

And here is the class:

 class DisposableList<T> : List<T>, IDisposable
  where T : IDisposable
 {
  public bool IsDisposed = false;
  public T BeginUsing(T item) { base.Add(item); return item; }
  public void Dispose()
  {
   for (int ix = this.Count - 1; ix >= 0; ix--)
   {
    try { this[ix].Dispose(); }
    catch (Exception e) { Logger.LogError(e); }
    this.RemoveAt(ix);
   }
   IsDisposed = true;
  }
 }