Should You Implement Idisposable.Dispose() So That It Never Throws

Should you implement IDisposable.Dispose() so that it never throws?

I would argue that swallowing is the lesser of the two evils in this scenario, as it is better to raise the original Exception - caveat: unless, perhaps the failure to cleanly dispose is itself pretty darned critical (perhaps if a TransactionScope couldn't dispose, since that could indicate a rollback failure).

See here for more thoughts on this - including a wrapper/extension method idea:

using(var foo = GetDodgyDisposableObject().Wrap()) {
   foo.BaseObject.SomeMethod();
   foo.BaseObject.SomeOtherMethod(); // etc
} // now exits properly even if Dispose() throws

Of course, you could also do some oddity where you re-throw a composite exception with both the original and second (Dispose()) exception - but think: you could have multiple using blocks... it would quickly become unmanageable. In reality, the original exception is the interesting one.

Should IDisposable.Dispose() implementations be idempotent?

should the implementation of the Dispose() method be idempotent

Yes, it should. There is no telling how many times it will be called.

From Implementing a Dispose Method on MSDN:

a Dispose method should be callable multiple times without throwing an exception.

An object with a good implementation of IDispose will have a boolean field flag indicating if it has been disposed of already and on subsequent calls do nothing (as it was already disposed).

What should I do when inheriting IDisposable with nothing to dispose?

You should only be throwing an exception when the caller is trying to perform an operation that cannot be performed because the object was disposed. If the operation they're trying to perform will work just fine even after the object is disposed, there's no reason to throw.

Of course, if you want to throw, you're certainly welcome to. It's your decision at the end of the day, but, unlike situations where the method/property has a dependency on a resource that is no longer there, there's just no longer a reason that pretty much forces you to throw.

Occasions when the using statement and IDisposable should never be used

I would say, always use using unless the documentation tells you not to (as in your example).

Having a Dispose method throw exceptions rather defeats the point of using it (pun intended). Whenever I implement it, I always try to ensure that no exceptions will be thrown out of it regardless of what state the object is in.

PS: Here's a simple utility method to compensate for WCF's behaviour. This ensures that Abort is called in every execution path other than when Close is called, and that errors are propogated up to the caller.

public static void CallSafely<T>(ChannelFactory<T> factory, Action<T> action) where T : class {
    var client = (IClientChannel) factory.CreateChannel();
    bool success = false;
    try {
        action((T) client);
        client.Close();
        success = true;
    } finally {
        if(!success) {
            client.Abort();
        }
    }
}

If you find any other funny behaviour cases elsewhere in the framework, you can come up with a similar strategy for handling them.

IDisposable.Dispose is never called after exception in using block

Environment.Exit will terminate the program

If Exit is called from a try or catch block, the code in any finally
block does not execute. If the return statement is used, the code in
the finally block does execute.

using (var x = new Disposable())
{
    throw new Exception("asdfsdf");
}

will be converted to

Disposable x = new Disposable();
try
{
    throw new Exception("asdfsdf");
}
finally
{
    if (x != null)
        x.Dispose();
}

Does a class need to implement IDisposable when all members are explicitly disposed?

For every instance of any type which implements IDisposable and might do so in a non-trivial fashion, it must at every moment be possible to identify how that instance will be Disposed. In most cases, this means that each IDisposable instance will have a well-defined owner, which is responsible for calling Dispose. In the case of the FileStream instance created by the class, your class is the owner, since nothing else will be able to Dispose it.

Classes with fields that references to IDisposable instances which they own should almost always implement IDisposable, and use their Dispose method to Dispose the IDisposable objects they own. Your class has such a field; it should thus implement IDisposable.

Whenever possible, a class which requires cleanup should be designed so that calling IDisposable.Dispose on it will suffice to perform any and all such cleanup as may be needed. In some cases, it may be impractical to perform cleanup without using some other method, but those cases are pretty rare. If one can design a class so that Dispose will take care of all necessary cleanup, one should do so.

Am I implementing IDisposable correctly?

You don't need to use this extensive version of IDisposable implementation if your class doesn't directly use unmanaged resources.

A simple

 public virtual void Dispose()
 {

     _Writer.Dispose();
 }

will suffice.

If your consumer fails to Dispose your object it will be GC'd normally without a call to Dispose, the object held by _Writer will also be GC'd and it will have a finaliser so it still gets to clean up its unmanaged resources properly.

Edit

Having done some research on the links provided by Matt and others I've come to the conclusion that my answer here stands. Here is why:-

The premise behind the disposable implementation "pattern" (by that I mean the protected virtual Dispose(bool), SuppressFinalize etc. marlarky) on an inheritable class is that a sub-class might hold on to an unmanaged resource.

However in the real world the vast majority of us .NET developers never go anywhere near an unmanaged resource. If you had to quantify the "might" above what probabilty figure would you come up with for you sort of .NET coding?

Lets assume I have a Person type (which for sake of argument has a disposable type in one of its fields and hence ought to be disposable itself). Now I have inheritors Customer, Employee etc. Is it really reasonable for me to clutter the Person class with this "Pattern" just in case someone inherits Person and wants to hold an unmanaged resource?

Sometimes we developers can over complicate things in an attempt to code for all possible circumstances without using some common sense regarding the relative probability of such circumstances.

If we ever wanted to use an unmanaged resource directly the sensible pattern would be wrap such a thing in its own class where the full "disposable pattern" would be reasonable. Hence in the significantly large body of "normal" code we do not to have to worry about all that mucking about. If we need IDisposable we can use the simple pattern above, inheritable or not.

Phew, glad to get that off my chest. ;)

What do you think of my IDisposable pattern implementation?

The implementation is severely flawed. You don't truly implement IDisposable, and you end up relying on the garbage collector to clean up your resources, which is a bad thing.

Additionally, you don't even clean up those resources properly when the GC does come around (it does do it correctly, but it's by mistake that it happens).

It is the responsibility of your class to implement IDisposable as you are holding onto references which implement IDisposable. Then, in your implementation of Dispose, if you are not being GCed (it is an explicit call to Dispose) you are to call Dispose on any IDisposable implementations that you are holding onto.

You check the connection status of the Socket, but that's not the same as calling Dispose on it, and you leak the resource as a result (GC eventually picks it up).

For the guideline on how to properly implement IDisposable, see the section of the MSDN documentation titled "Implementing Finalize and Dispose to Clean Up Unmanaged Resources", located here:

http://msdn.microsoft.com/en-us/library/b1yfkh5e(VS.71).aspx

I should note that I don't agree completely with these guidelines, but they are the most adopted. For my position, see here:

http://www.caspershouse.com/post/A-Better-Implementation-Pattern-for-IDisposable.aspx

---

Related Topics