Automatic Disposal Extension Method reasonable?

Question

I've been writing some custom WinForm controls which do a pretty heavy amount of drawing and therefore tend to have a lot of disposable graphics based fields lying around (Brushes, Pens, Bitmaps etc..) and as a result my Control's Dispose() method has to call Dispose on each of them.

I got concerned that I (or a future maintainer) could easily miss a field that needs to be disposed, either by forgetting to Dispose it or by not realising that it implements IDisposable. As such I wrote a very simple extension method on Object which finds all the IDisposable fields and disposes of them:

static public void DisposeAll(this Object obj)
{
   var disposable = obj.GetType()
                       .GetFields(BindingFlags.NonPublic | BindingFlags.Public | BindingFlags.Instance)
                       .Select(fi => fi.GetValue(obj))
                       .Where(o => o != null && o is IDisposable)
                       .Cast<IDisposable>();

   foreach (var d in disposable) d.Dispose();
}

My question is basically whether this is a reasonable thing to do. I can't think what it might screw up, but then I'm not particularly familiar with the inner workings of WinForms and this seems like the kind of thing (messing with reflection and disposing) that might cause irritating bugs down the line.

Answer 1

Usually you don't want to dispose all disposable members. E.g. a reference to the parent form.

Answer 2

I think this is dangerous. If you implement this idea, then the object will be disposed (if its disposable), and in certain situations like when you need the object to be present, it will be gone.

Automation is good for the objects which you are sure of that they will not be referred back at all.

Answer 3

You can use FxCop or Code Analysis (Visual Studio 2010 Premium or better).

Rule CA2213 will look for fields that you forgot to dispose.

Answer 4

It's tough to say if it will work for your particular case; I see greatest problem in order of Dispose() invocations. If one unmanaged handle depends on another you will have a weird problem with drawing, memory etc.

IMHO.

Answer 5

No, this is not a good approach. The drawing objects you create in an OnPaint method should always be local variables, wrapped by a using statement. Your approach requires declaring a class, just to store those objects. Painful and inefficient. Furthermore, you'll dispose objects that should never be disposed. Like the prefab pens and brushes, Pens.Black for example.