Multipule Operations in a finally block

Question

Lets say you had some resource clean up like: This is C#.

try{/*stuff*/}
catch(Exception e) {/*rollback logs etc.*/}
finally{
 if( context.Transaction != null )
  context.Transaction.Dispose();
 context.Connection.Close();
 context.Connection.Dispose();
}

Would it be more robust to do this instead?

try{/*stuff*/}
catch(Exception e) {/*rollback logs etc.*/}
finally{
 try{
  if( context.Transaction != null )
   context.Transaction.Dispose();
 }catch(Exception e){/*logs*/}
 finally{
  context.Connection.Close();
  context.Connection.Dispose();
 }
}

This way if the transaction.dispose manages to fail at leat the connection will be given the chance to close.

Answer 1

Would it be more robust to do this instead?

You would be better with multiple using blocks.

First, your catch blocks will eat all exceptions and are not needed (can have try ... finally without any catches). Only use catch if you can handle (or add value to) the exception.

But better:

using (var resA = GetMeAResourceNeedingCleanUp())
using (var resB = new AnotherResourceNeedingCleanUpn(...)) {
  // Code that might throw goes in here.
}

NB. Once an exception is winding back, and finally blocks are clearing up, throwing an other exception is likely to lead to (at best) confusion about which is being handled. This second guideline:

DO NOT throw exceptions from Dispose methods or finalizers. If you need to allow users to handle cleanup failures provide a separate Close method which can report its failure.

Note, the "Framework Design Guidelines" (2^nd ed) has this as (§9.4.1):

AVOID throwing an exception from within Dispose(bool) except under critical situations where the containing process has been corrupted (leaks, inconsistent shared state, etc.).

Answer 2

why would a Dispose call fail? You can also be too careful at some point. Like wrap every 'new' statement with a try/catch in case memory runs out...

Answer 3

Three points:

• You don't need to call Close as well as dispose
• Disposing of the transaction in a separate finally block is a better idea, as it guards against exceptions being thrown during disposal. (It shouldn't happen often, but it might do.)
• The using statement is almost always the cleanest way to dispose of resources. I use this even if I also want a try/catch block, simply because it's the idiomatic way of saying, "This uses a resource which I want to be disposed at the end of the block"

Combining these would lead to two using statements:

using (SqlConnection conn = ...)
{
    using (Transaction trans = ...)
    {
    }
}

If you want to avoid excessive indentation, you can write this as:

using (SqlConnection conn = ...)
using (Transaction trans = ...)
{
}

Answer 4

I don't like my finally clauses to be too verbose (or any clause, for that matter). I would refactor your resource cleanup into some utility class. Keep all the nested try's and "if null" conditions there, so you get better reuse. For example, because your cleanup logic resides in only one location, you can easily change your mind later about whether you really need to call Dispose().

Even more important, your application code becomes much more comprehensible.

try{/*stuff*/}
catch(Exception e) {/*rollback logs etc.*/}
finally{
  Utility.cleanup(context);
}