views:

181

answers:

5

Hi,

I was reading about this scenario where making use of the C# using statement can cause problems. Exceptions thrown within the scope of the using block can be lost if the Dispose function called at the end of the using statement was to throw an exception also. This highlights that care should be taken in certain cases when deciding whether to add the using statement.

I only tend to make use of using statements when using streams and the classes derived from DbConnection. If I need to clean up unmanaged resources, I would normally prefer to use a finally block.

This is another use of the IDisposable interface to create a performance timer that will stop the timer and log the time to the registry within the Dispose function. http://thebuildingcoder.typepad.com/blog/2010/03/performance-profiling.html

Is this good use of the IDisposable interface? It is not cleaning up resources or disposing of any further objects. However, I can see how it could clean up the calling code by wrapping the code it is profiling neatly in a using statement.

Are there times when the using statement and the IDisposable interface should never be used? Has implementing IDisposable or wrapping code in a using statement caused problems for you before?

Thanks

+5  A: 

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.

Christian Hayter
Precisely. Any `Dispose` method that throws an exception would be broken, in my book. It's analagous to throwing an exception from a C++ destructor (a no-no).
Stephen Cleary
A note on the opposite: the framework contains gazillion examples of situations where Dispose is implemented emptily, because the inheritance chain demands it (see also my answer for elaboration), in which cases you might consider abandoning using `using`.
Abel
@Abel: VERY BAD IDEA. Are you planning to use Reflector in every release to make sure they don't now actually _do_ something in Dispose?
John Saunders
Haha, no, of course not. But when was the last time you saw an implementation of IEnumerator and wrapped it in a using-block? And when was the last time you did the same for a Label or PlaceHolder control? Maybe a bad idea (I agree, see my own answer), but it happens all around us.
Abel
@Abel: A lot of `IEnumerator` clases do not implement `IDisposable`, but if they did I would certainly call `Dispose` on them or just use the `foreach` construct which calls `Dispose` automatically. Regarding controls...you do not need to in the mainstream scenarios because they are usually contained with another `Form` or `Control` acting as the container which will correctly call `Dispose` when necessary.
Brian Gideon
@Brian Gideon: good points (but `IEnumerator<>` descendants *always* implement `IDiposable`). I think I have been misunderstood a bit, as I'm personally in favor of using `using`. But sometimes it is just not feasible (long lived objects, i.e. in a session or cache) or not needed. An interesting side effect of this disucsion with Hans Passant who favors *not* using `using`: http://stackoverflow.com/questions/3257877/c-net-convert-icon-to-byte-and-back-again/3257928#3257928
Abel
@Abel: In the case of long lived objects (or objects whose scope is not limited to a single method) then obviously `using` is of little use, but just remember to call `Dispose` manually. By the way I did not see that Hans favors not using `using`, but I then again I was pretty confused by the exchange in that link to begin with.
Brian Gideon
+1  A: 

I think the bigger problem is throwing exceptions in Dispose. RAII patterns usually explicitly state that such should not be done, as it can create situations just like this one. I mean, what is the recovery path for something not disposing correctly, other than simply ending execution?

Also, it seems like this can be avoided with two try-catch statements:

try
{
    using(...)
    {
        try
        {
            // Do stuff
        }
        catch(NonDisposeException e)
        {
        }
    }
}
catch(DisposeException e)
{
}

The only problem that can occur here is if DisposeException is the same or a supertype of NonDisposeException, and you are trying to rethrow out of the NonDisposeException catch. In that case, the DisposeException block will catch it. So you might need some additional boolean marker to check for this.

jdmichal
I agree with your observation that throwing inside Dispose is not done. Your code, however, might be slightly more readable if you remove the using-block altogether and add a finally-statement. But that's a matter of style and opinion of course. Whatever you try, it remains ugly (which is why Dispose must not throw, nor must a finalizer!)
Abel
A: 

One example is the IAsyncResult.AsyncWaitHandle property. The astute programmer will recognize that WaitHandle classes implement IDisposable and naturally try to greedily dispose them. Except that most of the implementations of the APM in the BCL actually do lazy initialization of the WaitHandle inside the property. Obviously the result is that the programmer did more work than was necessary.

So where is the breakdown? Well, Microsoft screwed up the IAsyncResult inteface. Had they followed their own advice IAsyncResult would have derived from IDisposable since the implication is that it holds disposable resources. The astute programmer would then just call Dispose on the IAsyncResult and let it decide how best to dispose its constituents.

This is one of the classic fringe cases where disposing of an IDisposable could be problematic. Jeffrey Richter actually uses this example to argue (incorrectly in my opinion) that calling Dispose is not mandatory. You can read the debate here.

Brian Gideon
+1  A: 

The general rule of thumb is simple: when a class implements IDisposable, use using. When you need to catch errors, use try/catch/finally, to be able to catch the errors.

A few observations, however.

  1. You ask whether situations exist where IDisposable should not be used. Well: in most situations you shouldn't need to implement it. Use it when you want to free up resources timely, as opposed to waiting until the finalizer kicks in.

  2. When IDisposable is implemented, it should mean that the corresponding Dispose method clears its own resources and loops through any referenced or owned objects and calls Dispose on them. It should also flag whether Dispose is called already, to prevent multiple cleanups or referenced objects to do the same, resulting in an endless loop. However, all this is no guarantee that all references to the current object are gone, which means it will remain in memory until all references are gone and the finalizer kicks in.

  3. Throwing exceptions in Dispose is frowned upon and when it happens, state is possibly not guaranteed anymore. A nasty situation to be in. You can fix it by using try/catch/finally and in the finally block, add another try/catch. But like I said: this gets ugly pretty quickly.

  4. Using using is one thing, but don't confuse it with using try/finally. Both are equal, but the using-statement makes life easier by adding scoping and null-checks, which is a pain to do by hand each time. The using-statement translates to this (from C# standard):

    {
        SomeType withDispose = new SomeType();
        try
        {
             // use withDispose
        }            
        finally 
        {
            if (withDispose != null)
            {
                 ((IDisposable)withDispose).Dispose();
            }
        }
    }
    
  5. There are occasions where wrapping an object into a using-block is not necessary. These occasions are rare. They happen when you find yourself inheriting from an interface that inherits from IDisposable just in case a child would require disposing. An often-used example is IComponent, which is used with every Control (Form, EditBox, UserControl, you name it). And I rarely see people wrapping all these controls in using-statements. Another famous example is IEnumerator<T>. When using its descendants, one rarely sees using-blocks either.

Conclusion

Use the using-statement ubiquitously, and be judicious about alternatives or leaving it out. Make sure you know the implications of (not) using it, and be aware of the equality of using and try/finally. Need to catch anything? Use try/catch/finally.

Abel
A: 

The only case I know about is WCF clients. That's due to a design bug in WCF - Dispose should never throw exceptions. They missed that one.

John Saunders