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

Question

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?

Modifier

An exceptionally good question, I believe the answer is minimise their use and try to keep the high level IDisposables short lived with using, but this isn't always possible (especially where those IDisposables are due to interop with C++ dlls or similar). Look fwd to the answers. – Passim

Okay Dan, I've updated the question to show both methods of implementing IDisposable on Shoelace. – Modifier

I'm typically wary of relying on the implementation details of other classes to protect me. No point risking it if I can easily prevent it. Maybe I'm over-cautious or maybe I just spent too long as a C programmer, but I'd rather take the Irish approach: "To be sure, to be sure" :) – Modifier

@Dan: The null check is still needed to ensure that the object itself hasn't been set to null, in which case the call to waitHandle.Dispose() will throw a NullReferenceException. – Eulogia

No matter what, you should actually still use the Dispose(bool) method as shown in your "Implement IDisposable on Shoelace" section since that (minus the finalizer) is the full pattern. Just because a class implents IDisposable doesn't mean it needs a finalizer. – Eulogia

@Scott Dorman: I strongly disagree. The "dispose-pattern" is one of the worst things that MS ever came up with. Classes that own other disposable classes should typically a) NOT directly own unmanaged resources and b) be sealed. => Just implement Dispose(). And even if they're meant to be a base class, there's no reason to implement the dispose pattern - just implement Dispose virtual in that case. Because IMO there's no excuse (and no need) to extend a class with something that directly owns an unmanaged resource. – Tassel

Answer 1

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.

Lindsaylindsey

The advice that you can ignore it because the implementation today doesn't do anything is bad, because a future version might actually do something important in Dispose, and now you're got a hard to track down leak. – Ioyal

"keep expensive resources", IDisposable's are not necessarily expensive, indeed this example which uses an even wait handle is about a "light weight" as you get. – Persuade

Answer 2

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.

Extrasystole

It feels very strange to make an association there. This AutoResetEvent is private to the Shoelace implementation, so exposing it publicly would be wrong in my opinion. – Modifier

@GrahamS, i'm not saying expose it publically. I'm saying can it be moved to the point where shoelaces are tied. Perhaps if tying shoe laces is such an intricate task their should be a shoelace tier class. – Extrasystole

Could you expand your answer with some code snippets JaredPar? I may be stretching my example a little but I am imagining that Shoelace creates and starts Tyer thread which waits patiently at waitHandle.WaitOne() The Shoelace would call waitHandle.Set() when it wanted the thread to start tying. – Modifier

+1 on this. I think it would be strange that just Shoelace would be called concurrently but not the other ones. Think of what should be the aggregate root. In this case it should be Bus, IMHO, although i'm unfamiliar with the domain. So, in that case, the bus should contain the waithandle and all operations against the bus and all it 's children will be synchronized. – Requiem

Answer 3

To prevent IDisposable from spreading, you should try to encapsulate the use of a disposable object inside of a single method. Try to design Shoelace differently:

class Shoelace { 
  bool tied = false; 

  public void Tie() {

    using (var waitHandle = new AutoResetEvent(false)) {

      // you can even pass the disposable to other methods
      OtherMethod(waitHandle);

      // or hold it in a field (but FxCop will complain that your class is not disposable),
      // as long as you take control of its lifecycle
      _waitHandle = waitHandle;
      OtherMethodThatUsesTheWaitHandleFromTheField();

    } 

  }
} 

The scope of the wait handle is limited to the Tiemethod, and the class doesn't need to have a disposable field, and so won't need to be disposable itself.

Since the wait handle is an implementation detail inside of the Shoelace, it shouldn't change in any way its public interface, like adding a new interface in its declaration. What will happen then when you don't need a disposable field anymore, will you remove the IDisposable declaration? If you think about the Shoelace abstraction, you quickly realize that it shouldn't be polluted by infrastructure dependencies, like IDisposable. IDisposable should be reserved for classes whose abstraction encapsulate a resource that calls for deterministic clean up; i.e., for classes where disposability is part of the abstraction.

Metropolis

I completely agree that the implementation detail of Shoelace should not pollute the public interface, but my point is that it can be difficult to avoid. What you suggest here wouldn't always be possible: the purpose of the AutoResetEvent() is to communicate between threads, so its scope would usually extend beyond a single method. – Modifier

@GrahamS: that's why I said to try to design it that way. You can also pass the disposable to other methods. It only breaks down when external calls to the class control the lifecycle of the disposable. I'll update the answer accordingly. – Extrados

sorry, I know you can pass the disposable around, but I still don't see that working. In my example the AutoResetEvent is used to communicate between different threads that operate within the same class, so it has to be a member variable. You can't limit its scope to a method. (e.g. imagine that one thread just sits waiting for some work by blocking on waitHandle.WaitOne(). The main thread then calls the shoelace.Tie() method, which just does a waitHandle.Set() and returns immediately). – Modifier

Answer 4

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.

Waisted

Thanks Henk. If I took the creation of waitHandle out of Shoelace then someone still has to Dispose of it somewhere so how would that someone know that Shoelace is finished with it (and that Shoelace hasn't passed it to any other classes)? – Modifier

You'll need to move not just the Creation of but also the 'responsibility for' the waithandle. jaredPar's answer has an interesting start of an idea. – Waisted

This blog covers implementing IDisposable, and in particular addresses how to simplify the task by avoiding mixing managed and unmanaged resources in a single class blog.stephencleary.com/2009/08/… – Tartary

Answer 5

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.

Modifier

If Show implements IDisposable, creating it in a field initializer is dangerous. Interesting that FXCop wouldn't allow such initializations, since in C# the only way to do them safely is with ThreadStatic variables (downright hideous). In vb.net, it's possible to initialize IDisposable objects safely without ThreadStatic variables. The code is still ugly, but not quite so hideous. I wish MS would provide a nice way to use such field initializers safely. – Speer

@supercat: thanks. Can you explain why it is dangerous? I'm guessing that if an exception was thrown in one of the Shoe constructors then shoes would be left in a difficult state? – Modifier

Unless one knows that a particular type of IDisposable can be safely abandoned, one should ensure that every object that's created gets Dispose'd. If an IDisposable gets created in an object's field initializer but an exception gets thrown before the object is completely constructed, the object and all its fields will get abandoned. Even if the construction of the object is wrapped in a factory method which uses a "try" block to detect that construction failed, it's difficult for the factory to get references to the objects needing disposal. – Speer

The only way I know to deal with that in C# would be to use a ThreadStatic variable to keep a list of objects that will need disposal if the current constructor throws. Then have each field initializer register each object as it is created, have the factory clear the list if it completes successfully, and use a "finally" clause to Dispose all items from the list if it wasn't cleared. That would be a workable approach, but ThreadStatic variables are ugly. In vb.net, one could use a similar technique without needing any ThreadStatic variables. – Speer

Answer 6

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.

Merryman

This doesn't really solve the original problem though. It might keep FxCop quiet but something still should dispose the Shoelace. – Mishamishaan

I think that all you've done is move the problem elsewhere. ShoePairWithDisposableLaces now owns Shoelace(), so it must also be made IDisposable - so who disposes of the shoes? Are you leaving this to some IoC container to handle? – Modifier

@AndrewS: Indeed, something must still dispose of drivers, shoes and laces, but the disposal should not be initiated by buses. @GrahamS: You are right, I am moving the problem elsewhere, because I believe it belongs elsewhere. Typically, there would be a class instantiating shoes, which would also be responsible for disposing of shoes. I have edited the code to make ShoePairWithDisposableLaces disposable too. – Merryman

@Joh: Thanks. As you say, the issue with moving it elsewhere is that you create a whole lot more complexity. If ShoePairWithDisposableLaces is created by some other class, then wouldn't that class then have to be notified when the Driver has finished with his Shoes, so that it can properly Dispose() of them? – Modifier

@Graham: yes, a notification mechanism of some sort would be needed. A disposal policy based on reference counts could be used, for instance. There's some added complexity, but as you probably know, "There's no such thing as a free shoe" :) – Merryman

Answer 7

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))}));
        }
    }
}

Architect

Now you've made it the callers problem, and Shoelace can't depend on the wait handle being available when it needs it. – Ioyal

@andy What do you mean by "Shoelace can't depend on the wait handle being available when it needs it"? It's passed in to the constructor. – Architect

Something else might have messed with the autoresetevents state prior to giving it to Shoelace, and it might start off with the ARE in a bad state; something else might muck with the ARE's state while Shoelace is doing its thing, causing Shoelace to do the wrong thing. Its the same reason you only lock on private members. "In general, .. avoid locking on instances beyond your codes control" learn.microsoft.com/en-us/dotnet/csharp/language-reference/… – Ioyal

I agree with only locking on private members. But it seems like wait handles are different. In fact, it seems more useful to pass them in to the object as the same instance needs to be used to communicate across threads. Nonetheless, I think IoC provides a useful solution to the OP's problem. – Architect

Given that the OPs example had the waithandle as a private member, the safe assumption is that the object expects exclusive access to it. Making the handle visible outside the instance violates that. Typically an IoC can be good for things like this, but not when it comes to threading. – Ioyal

Also, Shoelace can escape using scope and resource will be Disposed while object using it still can be referenced. – Smack