104

Is there an elegant way to handle exceptions that are thrown in 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?

bluish
  • 26,356
  • 27
  • 122
  • 180
Paul
  • 1,757
  • 2
  • 11
  • 21

15 Answers15

74

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 );
  }
}
Dave Jarvis
  • 30,436
  • 41
  • 178
  • 315
Darron
  • 21,309
  • 5
  • 49
  • 53
  • 5
    Yeap, I use a very similar idiom. But I don't create a function for that. – OscarRyz Jan 26 '09 at 22:42
  • 9
    A function is handy if you need to use the idiom a few places in the same class. – Darron Jan 27 '09 at 12:13
  • 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 Feb 15 '10 at 18:04
  • Logging is a cross-cutting concern and best left out of code directly. Indirectly apply it using aspects. – Dave Jarvis Feb 15 '10 at 18:10
  • 14
    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 Feb 17 '10 at 15:09
  • 1
    `Resource` => `Closeable`? – Dmitry Ginzburg Jun 23 '15 at 09:28
  • I would recommend using _try-with-resources_ whenever possible. With this, if some exception is thrown while closing the resource, it will be marked as Suppressed exception and you will not lose the main exception thrown from the try block. Also, i think you get these suppressed exceptions in your stack trace, so they are not lost too. – abhisheknirmal May 04 '16 at 14:11
26

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
    }
}
bluish
  • 26,356
  • 27
  • 122
  • 180
CJS
  • 1,455
  • 1
  • 13
  • 17
22

If you're using Java 7, and resource implements AutoClosable, you can do this (using InputStream as an example):

try (InputStream resource = getInputStream()) {
  // Use the resource.
}
catch( Exception ex ) {
  // Problem with the resource.
}
Kevin Wong
  • 14,656
  • 11
  • 42
  • 52
8

Arguably a bit over the top, but maybe useful if you're letting exceptions bubble up and you can't log anything from within your method (e.g. because it's a library and you'd rather let the calling code handle exceptions and logging):

Resource resource = null;
boolean isSuccess = false;
try {
    resource = Resource.create();
    resource.use();
    // Following line will only run if nothing above threw an exception.
    isSuccess = true;
} finally {
    if (resource != null) {
        if (isSuccess) {
            // let close throw the exception so it isn't swallowed.
            resource.close();
        } else {
            try {
                resource.close();
            } catch (ResourceException ignore) {
                // Just swallow this one because you don't want it 
                // to replace the one that came first (thrown above).
            }
        }
    }
}

UPDATE: I looked into this a bit more and found a great blog post from someone who has clearly thought about this more than me: http://illegalargumentexception.blogspot.com/2008/10/java-how-not-to-make-mess-of-stream.html He goes one step further and combines the two exceptions into one, which I could see being useful in some cases.

MB.
  • 7,365
  • 6
  • 42
  • 42
6

As of Java 7 you no longer need to explicitly close resources in a finally block instead you can use try-with-resources syntax. The try-with-resources statement is a try statement that declares one or more resources. A resource is an object that must be closed after the program is finished with it. The try-with-resources statement ensures that each resource is closed at the end of the statement. Any object that implements java.lang.AutoCloseable, which includes all objects which implement java.io.Closeable, can be used as a resource.

Assume the following code:

try( Connection con = null;
     Statement stmt = con.createStatement();
     Result rs= stmt.executeQuery(QUERY);)
{  
     count = rs.getInt(1);
}

If any exception happens the close method will be called on each of these three resources in opposite order in which they were created. It means the close method would be called first for ResultSetm then the Statement and at the end for the Connection object.

It's also important to know that any exceptions that occur when the close methods is automatically called are suppressed. These suppressed exceptions can be retrieved by getsuppressed() method defined in the Throwable class.

Source: https://docs.oracle.com/javase/tutorial/essential/exceptions/tryResourceClose.html

Soroosh
  • 477
  • 2
  • 7
  • 18
  • It seems incomplete that this answer doesn't mention the difference in behavior between this approach and how the OP's posted example code works. – Nathan Hughes Apr 09 '15 at 16:59
  • 2
    using try-with-resources throws an exception on close if the part in the try block completes normally but the close method doesn't, unlike what the OP code does. recommending it as a replacement without acknowledging the change in behavior seems potentially misleading. – Nathan Hughes Apr 09 '15 at 17:21
  • It doesn't throws an exception , close method is automatically called are suppressed. – Soroosh Apr 09 '15 at 17:28
  • 2
    try the case i described. try block completes normally, close throws something. and reread the page you posted the link to, suppression only applies when the try block throws something. – Nathan Hughes Apr 09 '15 at 17:30
3

Ignoring exceptions which occur in a 'finally' block is generally a bad idea unless one knows what those exceptions will be and what conditions they will represent. In the normal try/finally usage pattern, the try block places things into a state the outside code won't be expecting, and the finally block restores those things' state to what the outside code expects. Outside code which catches an exception will generally expect that, despite the exception, everything has been restored to a normal state. For example, suppose some code starts a transaction and then tries to add two records; the "finally" block performs a "rollback if not committed" operation. A caller might be prepared for an exception to occur during the execution of the second "add" operation, and may expect that if it catches such an exception, the database will be in the state it was before either operation was attempted. If, however, a second exception occurs during the rollback, bad things could happen if the caller makes any assumptions about the database state. The rollback failure represents a major crisis--one which should not be caught by code expecting a mere "Failed to add record" exception.

My personal inclination would be to have a finally method catch exceptions that occur and wrap them in a "CleanupFailedException", recognizing that such failure represents a major problem and such an exception should not be caught lightly.

supercat
  • 77,689
  • 9
  • 166
  • 211
2

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
  • 34,472
  • 31
  • 113
  • 192
1

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
  • 9,785
  • 9
  • 61
  • 73
  • 2
    -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. – Tim Frey Jan 26 '09 at 22:16
  • 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 Jan 26 '09 at 22:18
1

After lots of consideration, I find the following code best:

MyResource resource = null;
try {
    resource = new MyResource();
    resource.doSomethingFancy();
    resource.close(); 
    resource = null;  
} finally {
    closeQuietly(resource)
}

void closeQuietly(MyResource a) {
    if (a!=null)
        try {
             a.close();
        } catch (Exception e) {
             //ignore
        }
}

That code guarantees following:

  1. The resource is freed when the code finished
  2. Exceptions thrown when closing the resource are not consumed without processing them.
  3. The code does not try to close the resource twice, no unnecessary exception will be created.
Grogi
  • 2,099
  • 17
  • 13
  • You could also avoid calling resource.close(); resource = null in the try block, that is what finally blocks are for. Also note that you do not handle any exceptions thrown while "doing something fancy", which actually, I think I prefer better, to handle infrastructural exceptions at a higher application level down the stack. – Paul Feb 04 '12 at 09:36
  • The resource.close() might throw and exception as well - i.e. when the buffer flush fails. This exception should never be consumed. However, if closing the stream as a result of previously raised exception, the resource should be quietly closed ignoring the exception and preserving the root cause. – Grogi Apr 24 '13 at 13:19
0

Changing Resource from best answer to Closeable

Streams implements Closeable Thus you can reuse the method for all streams

protected void closeQuietly(Closeable resource) {
    if (resource == null) 
        return;
    try {
        resource.close();
    } catch (IOException e) {
        //log the exception
    }
}
Ryan
  • 130
  • 11
0

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

Ken Henderson
  • 2,828
  • 1
  • 18
  • 16
  • Testing error conditions is in general a good practice, simply because exceptions are expensive. – Dirk Vollmar Jan 26 '09 at 21:45
  • "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 Jan 26 '09 at 21:49
  • @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. – Ken Henderson Jan 26 '09 at 21:56
  • 1
    -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. – Tim Frey Jan 26 '09 at 22:14
  • @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). – Ken Henderson Jan 28 '09 at 21:36
  • @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 May 20 '10 at 20:17
  • 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 May 20 '10 at 20:40
0

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
  • 128,308
  • 78
  • 326
  • 506
0

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
  • 196,001
  • 113
  • 385
  • 569
  • 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 Jan 26 '09 at 22:49
  • @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 Jan 27 '09 at 05:13
0
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
  • 145,806
  • 30
  • 211
  • 305
  • 5
    What if use(resource) throws Exception A and then resource.release() throws exception B? Exception A is lost... – Darron Jan 27 '09 at 14:15
0

I encountered a situation similar where I couldn't use try with resources but I also wanted to handle the exception coming from the close, not just log and ignore it like closeQuietly mechanism do. in my case I'm not actually dealing with an output stream, so the failure on close is of more interest than a simple stream.

IOException ioException = null;
try {
  outputStream.write("Something");
  outputStream.flush();
} catch (IOException e) {
  throw new ExportException("Unable to write to response stream", e);
}
finally {
  try {
    outputStream.close();
  } catch (IOException e) {
    ioException = e;
  }
}
if (ioException != null) {
  throw new ExportException("Unable to close outputstream", ioException);
}