views:

4058

answers:

8

Is there an elegant way to handle exceptions that are thrown in the a finally block?

For example:

try {
  // Use the resource.
}
catch( Exception ex ) {
  // Problem with the resource.
}
finally {
   try{
     resource.close();
   }
   catch( Exception ex ) {
     // Could not close the resource?
   }
}

How do you avoid the try/catch in the finally block?

+1  A: 

One solution, if the two Exceptions are two different classes

try {
    ...
    }
catch(package1.Exception err)
   {
    ...
   }
catch(package2.Exception err)
   {
   ...
   }
finally
  {
  }

But sometimes you cannot avoid this second try-catch. e.g. for closing a stream

InputStream in=null;
try
 {
 in= new FileInputStream("File.txt");
 (..)// do something that might throw an exception during the analysis of the file, e.g. a SQL error
 }
catch(SQLException err)
 {
 //handle exception
 }
finally
 {
 //at the end, we close the file
 if(in!=null) try { in.close();} catch(IOException err) { /* ignore */ }
 }
Pierre
In your case if you used a "using" statement, it should clean up the resource.
Chuck Conway
My bad, I'm assuming it's C#.
Chuck Conway
A: 

If you can you should test to avoid the error condition to begin with.

try{...}
catch(NullArgumentException nae){...}
finally
{
  //or if resource had some useful function that tells you its open use that
  if (resource != null) 
  {
      resource.Close();
      resource = null;//just to be explicit about it was closed
  }
}

Also you should probably only be catching exceptions that you can recover from, if you can't recover then let it propagate to the top level of your program. If you can't test for an error condition that you will have to surround your code with a try catch block like you already have done (although I would recommend still catching specific, expected errors).

confusedGeek
Testing error conditions is in general a good practice, simply because exceptions are expensive.
0xA3
"Defensive Programming" is an outmoded paradigm. The bloated code which results from testing for all error conditions eventually causes more problems than it solves. TDD and handling exceptions is that modern approach IMHO
Joe Soul-bringer
@Joe - I don't disagree you on testing for all error conditions, but sometimes it makes sense, especially in light of the difference (normally) in cost of a simple check to avoid the exception versus the exception itself.
confusedGeek
-1 Here, resource.Close() can throw an exception. If you need to close additional resources, the exception would cause the function to return and they will remain open. That's the purpose of the second try/catch in the OP.
Outlaw Programmer
pointless setting it to be =null
Egwor
@Outlaw - you're are missing my point if Close throws an exception, and the resource is open then by capturing and suppressing the exception how do I correct the problem? Hence why I let it propagate (its fairly rare that I can recover with it still open).
confusedGeek
@Egwor: It's not always pointless to set it to null. i.e. A huge bitmap would hang out in memory until you exit the scope, even after disposing it, because the lingering reference to the bitmap would prevent it and its managed resources from being garbage collected. It's a kind of *temporary* memory leak, where you're done using something, but can't reclaim its memory for use until that reference is gone and the GC is able to collect it. In this case, it's probably pointless... but the faster you nullify references, the sooner the (possibly scarce) resources can be collected and reused.
Triynko
See http://stackoverflow.com/questions/680550/explicit-nulling . There's mentioning that the C# GC is smart enough to consider "last possible reference" too, which would tend to make explicit-nulling pointless.
Triynko
+1  A: 

Why do you want to avoid the additional block? Since the finally block contains "normal" operations which may throw an exception AND you want the finally block to run completely you HAVE to catch exceptions.

If you don't expect the finally block to throw an exception and you don't know how to handle the exception anyway (you would just dump stack trace) let the exception bubble up the call stack (remove the try-catch from the finally block).

If you want to reduce typing you could implement a "global" outer try-catch block, which will catch all exceptions thrown in finally blocks:

try {
    try {
        ...
    } catch (Exception ex) {
        ...
    } finally {
        ...
    }

    try {
        ...
    } catch (Exception ex) {
        ...
    } finally {
        ...
    }

    try {
        ...
    } catch (Exception ex) {
        ...
    } finally {
        ...
    }
} catch (Exception ex) {
    ...
}
Eduard Wirch
-1 For this one, too. What if you're trying to close multiple resources in a single finally block? If closing the first resource fails, the others will remain open once the exception is thrown.
Outlaw Programmer
This is why I told Paul that you HAVE to catch exceptions if you want to make sure the finally block completes. Please read the WHOLE answer!
Eduard Wirch
A: 

You could refactor this into another method ...

public void RealDoSuff()
{
   try
   { DoStuff(); }
   catch
   { // resource.close failed or something really weird is going on 
     // like an OutOfMemoryException 
   }
}

private void DoStuff() 
{
  try 
  {}
  catch
  {
  }
  finally 
  {
    if (resource != null) 
    {
      resource.close(); 
    }
  }
}
Sam Saffron
+6  A: 

I usually do it like this:

try {
  // Use the resource.
} catch( Exception ex ) {
  // Problem with the resource.
} finally {
  // Put away the resource.
  closeQuietly( resource );
}

Elsewhere:

protected void closeQuietly( Resource resource ) {
  try {
    if (resource != null) {
      resource.close();
    }
  } catch( Exception ex ) {
    log( "Exception during Resource.close()", ex );
  }
}
Darron
Yeap, I use a very similar idiom. But I don't create a function for that.
OscarRyz
A function is handy if you need to use the idiom a few places in the same class.
Darron
The check for null is redundant. If the resource was null, then the calling method is broken should be fixed. Also, if the resource is null, that should probably be logged. Otherwise it results in a potential exception being silently ignored.
Dave Jarvis
Logging is a cross-cutting concern and best left out of code directly. Indirectly apply it using aspects.
Dave Jarvis
The check for null is not always redundant. Think of "resource = new FileInputStream("file.txt")" as the first line of the try.Also, this question was not about aspect oriented programing which many people do not use. However, the concept that the Exception should not be just ignored was most compactly handled by showing a log statement.
Darron
A: 

I usually do this:

MyResource r = null;
try { 
   // use resource
} finally {   
    if( r != null ) try { 
        r.close(); 
    } catch( ThatSpecificExceptionOnClose teoc ){}
}

Rationale: If I'm done with the resource and the only problem I have is closing it, there is not much I can do about it. It doesn't make sense either to kill the whole thread if I'm done with the resource anyway.

This is one of the cases when at least for me, it is safe to ignore that checked exception.

To this day I haven't had any problem using this idiom.

OscarRyz
I'd log it, just in case you find some leaks in the future. That way you'd know where they might (not) be coming from
Egwor
@Egwor. I agree with you. This was just some quick smippet. I log it too and probaly use a catch is something could be done with the exception :)
OscarRyz
+3  A: 

I typically use one of the closeQuietly methods in org.apache.commons.io.IOUtils:

public static void closeQuietly(OutputStream output) {
    try {
        if (output != null) {
            output.close();
        }
    } catch (IOException ioe) {
        // ignore
    }
}
CJS
You can make this method more general with Closeablepublic static void closeQuietly(Closeable closeable) {
Peter Lawrey
Yes, Closeable is nice. It's a shame that many things (like JDBC resources) don't implement it.
Darron
A: 
try {
    final Resource resource = acquire();
    try {
        use(resource);
    } finally {
        resource.release();
    }
} catch (ResourceException exx) {
    ... sensible code ...
}

Job done. No null tests. Single catch, include acquire and release exceptions. Of course you can use the Execute Around idiom and only have to write it once for each resource type.

Tom Hawtin - tackline
What if use(resource) throws Exception A and then resource.release() throws exception B? Exception A is lost...
Darron