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.

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?

5 Answers
5

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.

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 ClearyJul 20 '10 at 17:49

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.
–
AbelJul 20 '10 at 18:19

2

@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 SaundersJul 20 '10 at 18:23

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.
–
AbelJul 20 '10 at 18:31

1

@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 GideonJul 20 '10 at 18:35

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.

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.

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.

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.

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):

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.

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:

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.

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!)
–
AbelJul 20 '10 at 18:14

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.

Objects that own IDisposable objects should dispose them. An object which creates IDisposable object Foo would be expected to be the initial owner of Foo, but very odd to expect it to be the owner of Foo.SomeProperty unless it did something to explicitly acquire such ownership (e.g. call a CreateWaitHandle method which then indicated whether the caller should assume ownership of the WaitHandle in question). I could see a basis for trying to cast an IAsyncResult to IDisposable and calling Dispose if non-null (such a pattern is appropriate for non-generic IEnumerator...
–
supercatOct 29 '12 at 14:45

...as well). Otherwise, I see no basis for anything other than the IAsyncResult to assume ownership of its WaitHandle.
–
supercatOct 29 '12 at 14:45

@supercat: I agree. It should "own" that WaitHandle. The rub is that because IAsyncResult does not implement IDisposable it forces the programmer to choose between only 2 bad options: 1) leave the underlying WaitHandle undisposed or 2) assume ownership and attempt to greedily dispose it. #1 is bad because it would leave a system resource active for longer than necessary. #2 is bad because it deviates from standard practice and it may force the allocation of the resource when it otherwise wouldn't be needed because it's initialized lazily. Thus, Jeffrey Richter's argument is wrong.
–
Brian GideonOct 29 '12 at 18:29

You omit choice #3 which I alluded to above: test whether a particular IAsyncResult-implementing object implements IDisposable and call Dispose on it if so, and figure that any implementation of IAsyncResult which does not implement IDisposable must have its AsyncWaitHandle property return only types for which finalizers can provide adequate cleanup. If Mr. Richter's argument is that there exist some poorly-designed classes which implement IDisposable, whose instances should be abandoned without calling Dispose, I'd agree with him 100%; StreamReader is a prime example.
–
supercatOct 29 '12 at 18:48

If one wanted to argue that there not any well-designed classes which implement IDisposable, whose instances should be abandoned without disposal, I'd be more inclined to agree with that, though not 100%. Some types of objects implement IEnumerator<T> but don't use any resources. If code explicitly creates an instance of such a type (as opposed to receiving one via GetEnumerator() call, and if tracking the lifetime of such instances would be difficult, abandoning the instance may be better than complicating the code in an effort to assure disposal.
–
supercatOct 29 '12 at 19:01