How do you prevent IDisposable from spreading to all your classes?

0 votes
asked Mar 19, 2009 by grahams

Start with these simple classes...

Let's say I have a simple set of classes like this:

class Bus
{
    Driver busDriver = new Driver();
}

class Driver
{
    Shoe[] shoes = { new Shoe(), new Shoe() };
}

class Shoe
{
    Shoelace lace = new Shoelace();
}

class Shoelace
{
    bool tied = false;
}

A Bus has a Driver, the Driver has two Shoes, each Shoe has a Shoelace. All very silly.

Add an IDisposable object to Shoelace

Later I decide that some operation on the Shoelace could be multi-threaded, so I add an EventWaitHandle for the threads to communicate with. So Shoelace now looks like this:

class Shoelace
{
    private AutoResetEvent waitHandle = new AutoResetEvent(false);
    bool tied = false;
    // ... other stuff ..
}

Implement IDisposable on Shoelace

But now Microsoft's FxCop will complain: "Implement IDisposable on 'Shoelace' because it creates members of the following IDisposable types: 'EventWaitHandle'."

Okay, I implement IDisposable on Shoelace and my neat little class becomes this horrible mess:

class Shoelace : IDisposable
{
    private AutoResetEvent waitHandle = new AutoResetEvent(false);
    bool tied = false;
    private bool disposed = false;

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

    ~Shoelace()
    {
        Dispose(false);
    }

    protected virtual void Dispose(bool disposing)
    {
        if (!this.disposed)
        {
            if (disposing)
            {
                if (waitHandle != null)
                {
                    waitHandle.Close();
                    waitHandle = null;
                }
            }
            // No unmanaged resources to release otherwise they'd go here.
        }
        disposed = true;
    }
}

Or (as pointed out by commenters) since Shoelace itself has no unmanaged resources, I might use the simpler dispose implementation without needing the Dispose(bool) and Destructor:

class Shoelace : IDisposable
{
    private AutoResetEvent waitHandle = new AutoResetEvent(false);
    bool tied = false;

    public void Dispose()
    {
        if (waitHandle != null)
        {
            waitHandle.Close();
            waitHandle = null;
        }
        GC.SuppressFinalize(this);
    }
}

Watch in horror as IDisposable spreads

Right that's that fixed. But now FxCop will complain that Shoe creates a Shoelace, so Shoe must be IDisposable too.

And Driver creates Shoe so Driver must be IDisposable. And Bus creates Driver so Bus must be IDisposable and so on.

Suddenly my small change to Shoelace is causing me a lot of work and my boss is wondering why I need to checkout Bus to make a change to Shoelace.

The Question

How do you prevent this spread of IDisposable, but still ensure that your unmanaged objects are properly disposed?

7 Answers

0 votes
answered Mar 19, 2009 by grzenio

You can't really "prevent" IDisposable from spreading. Some classes need to be disposed, like AutoResetEvent, and the most efficient way is to do it in the Dispose() method to avoid the overhead of finalizers. But this method must be called somehow, so exactly as in your example the classes that encapsulate or contain IDisposable have to dispose these, so they have to be disposable as well, etc. The only way to avoid it is to:

  • avoid using IDisposable classes where possible, lock or wait for events in single places, keep expensive resources in single place, etc
  • create them only when you need them and dispose them just after (the using pattern)

In some cases IDisposable can be ignored because it supports an optional case. For example, WaitHandle implements IDisposable to support a named Mutex. If a name is not being used, the Dispose method does nothing. MemoryStream is another example, it uses no system resources and its Dispose implementation also does nothing. Careful thinking about whether an unmanaged resource is being used or not can be instructional. So can examining the available sources for the .net libraries or using a decompiler.

0 votes
answered Mar 19, 2009 by henk-holterman

This is basically what happens when you mix Composition or Aggregation with Disposable classes. As mentioned, the first way out would be to refactor the waitHandle out of shoelace.

Having said that, you can strip down the Disposable pattern considerably when you don't have unmanaged resources. (I'm still looking for an official reference for this.)

But you can omit the destructor and GC.SuppressFinalize(this); and maybe cleanup the virtual void Dispose(bool disposing) a bit.

0 votes
answered Mar 19, 2009 by jaredpar

In terms of correctness, you can't prevent the spread of IDisposable through an object relationship if a parent object creates and essentially owns a child object which must now be disposable. FxCop is correct in this situation and the parent must be IDisposable.

What you can do is avoid adding an IDisposable to a leaf class in your object hierarchy. This is not always an easy task but it's an interesting exercise. From a logical perspective, there is no reason that a ShoeLace needs to be disposable. Instead of adding a WaitHandle here, is it also possible to add an association between a ShoeLace and a WaitHandle at the point it's used. The simplest way is through an Dictionary instance.

If you can move the WaitHandle into a loose association via a map at the point the WaitHandle is actually used then you can break this chain.

0 votes
answered Mar 19, 2009 by adam

This feels a lot like a higher-level design issue, as is often the case when a "quick fix" devolves into a quagmire. For more discussion of ways out, you might find this thread helpful.

0 votes
answered Mar 19, 2009 by grahams

Interestingly if Driver is defined as above:

class Driver
{
    Shoe[] shoes = { new Shoe(), new Shoe() };
}

Then when Shoe is made IDisposable, FxCop (v1.36) does not complain that Driver should also be IDisposable.

However if it is defined like this:

class Driver
{
    Shoe leftShoe = new Shoe();
    Shoe rightShoe = new Shoe();
}

then it will complain.

I suspect that this is just a limitation of FxCop, rather than a solution, because in the first version the Shoe instances are still being created by the Driver and still need to be disposed somehow.

0 votes
answered Mar 6, 2010 by joh

I don't think there is a technical way of preventing IDisposable from spreading if you keep your design so tightly coupled. One should then wonder if the design is right.

In your example, I think it makes sense to have the shoe own the shoelace, and maybe, the driver should own his/her shoes. However, the bus should not own the driver. Typically, bus drivers do not follow buses to the scrapyard :) In the case of drivers and shoes, drivers seldom make their own shoes, meaning they don't really "own" them.

An alternative design could be:

class Bus
{
   IDriver busDriver = null;
   public void SetDriver(IDriver d) { busDriver = d; }
}

class Driver : IDriver
{
   IShoePair shoes = null;
   public void PutShoesOn(IShoePair p) { shoes = p; }
}

class ShoePairWithDisposableLaces : IShoePair, IDisposable
{
   Shoelace lace = new Shoelace();
}

class Shoelace : IDisposable
{
   ...
}

The new design is unfortunately more complicated, as it requires extra classes to instantiate and dispose of concrete instances of shoes and drivers, but this complication is inherent to the problem being solved. The good thing is that buses need no longer be disposable simply for the purpose of disposing of shoelaces.

0 votes
answered Mar 2, 2014 by darragh

How about using Inversion of Control?

class Bus
{
    private Driver busDriver;

    public Bus(Driver busDriver)
    {
        this.busDriver = busDriver;
    }
}

class Driver
{
    private Shoe[] shoes;

    public Driver(Shoe[] shoes)
    {
        this.shoes = shoes;
    }
}

class Shoe
{
    private Shoelace lace;

    public Shoe(Shoelace lace)
    {
        this.lace = lace;
    }
}

class Shoelace
{
    bool tied;
    private AutoResetEvent waitHandle;

    public Shoelace(bool tied, AutoResetEvent waitHandle)
    {
        this.tied = tied;
        this.waitHandle = waitHandle;
    }
}

class Program
{
    static void Main(string[] args)
    {
        using (var leftShoeWaitHandle = new AutoResetEvent(false))
        using (var rightShoeWaitHandle = new AutoResetEvent(false))
        {
            var bus = new Bus(new Driver(new[] {new Shoe(new Shoelace(false, leftShoeWaitHandle)),new Shoe(new Shoelace(false, rightShoeWaitHandle))}));
        }
    }
}
Welcome to Q&A, where you can ask questions and receive answers from other members of the community.
Website Online Counter

...