Title: Handling Exceptions in finally

Recently I was reviewing some old Java code that performs SQL queries against a database and closes the resources in finally blocks. When I examined the code I realized that the handling of the resources was flawed if an exception occurred. This article looks at how the handling of the resources and exceptions was problematic and some approaches to solving these problems.

The code in question was made up of static methods where each method used a Connection parameter and performed the necessary actions to create a query, perform the query and process the results of the query. My problem came from the handling of the query and results resources, i.e. the instances of PreparedStatement and ResultSet.

The PreparedStatement and ResultSet were created in the main try block of the method and were closed in the associated finally block. The close() method of these classes can throw a SQLException and in the finally block each close() method was wrapped in a try/catch where the SQLException was caught and converted into a RuntimeException to be thrown. The outline of the original code is shown in the following listing:

public static ArrayList foo(Connection conn)
                        throws SQLException {
  ArrayList results = null;
  PreparedStatement ps = null;
  ResultSet rs = null;
  try {   
    // create a query, perform the query and
    // process the results
  }
  finally {
    try {
      rs.close();
    }
    catch(SQLException ex) {
      throw new RuntimeException(ex);
    }
    try {
      ps.close();
    }
    catch(SQLException ex) {
      throw new RuntimeException(ex);
    }
  }
  return results;
}

There are a number of problems with this code:

We can fix some of the problems very easily by nesting the handling of the resources in try/finally blocks (as demonstrated in [Griffiths]) and to remove the conversion to RuntimeExceptions. This would be implemented in the method as follows:

// assign query to ps
try {   
  // perform the query and assign result to rs
  try {
    // process the results 
  }
  finally { rs.close(); }
}
finally { ps.close(); }

This solves the second problem, as the method is already declared to throw a SQLException no conversion is required, and the third problem, because even if a exception is thrown by rs.close() the ps.close() will always be called.

However this leaves the first problem of the lost exception. The suggested approach in [Warren-] is to "Never let exceptions propagate out of a finally block", this would be implemented in the finally block as follows:

finally {
  try {
    rs.close();
  }
  catch(SQLException ex) {
    /* exception ignored */
  }
  try {
    ps.close();
  }
  catch(SQLException ex) {
    /* exception ignored */
  }
}

This approach only solves the hidden exception problem in the original code but as a consequence adds an additional problem: it is possible for the rs.close() to be the original exception and this is ignored. Ignoring an exception is likely to make recovery in higher levels of the code more difficult, if not impossible. It is also likely to mislead a user trying to determine the cause of a failure; a later related exception may be mistakenly diagnosed as the source of the problem. The consequences of ignoring exceptions are discussed further in [Bloch] "Item 47: Don't ignore exceptions".

[Hagger] offers a different solution to this problem by collecting up the exceptions thrown during processing of a method. This is achieved by the creation of a derived exception class containing a collection of other exceptions (a slightly modified version follows):

class FooException extends Exception {
  private ArrayList exceptions;
  public FooException(ArrayList exs) {
    exceptions = exs;
  }
  public ArrayList getExceptions() {
    return exceptions;
  }
}

And the original code is modified to make use of this exception:

public static ArrayList foo(Connection conn)
           throws FooException {
  ArrayList exceptions = new ArrayList(); 
  ArrayList results = null;
  PreparedStatement ps = null;
  ResultSet rs = null;
  try {   
    // create a query, perform the query and
    // process the results
  }
  catch(SQLException ex) {
    exceptions.add(exception);
  }
  finally {
    try {
      rs.close();
    }
    catch(SQLException ex) {
      exceptions.add(ex);
    }
    try {
      ps.close();
    }
    catch(SQLException ex) {
      exceptions.add(ex);
    }
    if(exceptions.size() != 0) {
      throw new FooException(exceptions);
    }
  }
  return results;
}

This approach doesn't lose any of the exceptions thrown and the PreparedStatement will be closed even if the close of the ResultSet throws an exception, but now the method throws a user-defined Exception instead of SQLException. It is better to use standard exceptions where possible as discussed in [Bloch] "Item 42: Favor the use of standard exceptions". More importantly the exceptions are collected as peers not as causes, and so is not idiomatic (at least not since JDK1.4) where the Throwable class allows nesting of another Throwable as a cause [ JDK14].

SQLException was written before JDK1.4 and has its own mechanism for nesting other SQLExceptions, this is supported by methods setNextException() and getNextException(). This mechanism, being limited to SQLException, is not generally idiomatic for all Throwable types and so will be not be considered for the purposes of this article.

So a Throwable (and its derived classes) can be constructed with a cause (if this support has been implemented), or can be initialized with a cause using the initCause() method. Nesting exceptions at different levels of abstraction has been idiomatic even before support was added to Throwable, an implementation of this can be found at http://www.javaworld.com/javaworld/javatips/jw-javatip91.html. So to be more idiomatic the same approach should be taken within the original method.

We can use a modified version of Hagger's solution, combining this with nested try/finally blocks from the first solution and nest the SQLExceptions using initCause(), if required. Thus the original code is rewritten:

public static ArrayList foo(Connection conn)
          throws SQLException {
  SQLException cachedException = null;
  ArrayList results = null;
  PreparedStatement ps = null;
  ResultSet rs = null;
  // assign query to ps
  try {
    // perform query and assign result to rs
    try {
      // process the results 
    }
    catch(SQLException ex) {
      cachedException = ex;
      throw ex;
    }
    finally {
      try {
        rs.close();
      }
      catch(SQLException ex) {
        if(cachedException != null) {
          ex.initCause(cachedException);
        }
        cachedException = ex;
        throw ex;
      }
    }
  }
  catch(SQLException ex) {
    if(cachedException != null) {
      ex.initCause(cachedException);
    }
    cachedException = ex;
    throw ex;
  }
  finally {
    try {
      ps.close();
    }
    catch(SQLException ex) {
      if(cachedException != null) {
        ex.initCause(cachedException);
      }
      throw ex;
    }
  }
  return results;
}

This solves the three problems of the original code, no exception is lost, the exception thrown is a SQLException and the PreparedStatement is closed even if the attempt to close the ResultSet results in an Exception. Unfortunately this isn't a general solution, the initCause() method is used to set the cause of a SQLException if an existing SQLException had been caught, but initCause() has some restrictions:

This means that if the exceptions caught in the finally block already have a cause then the initCause() method call will fail with a java.lang.IllegalStateException. To explain further this example demonstrates how to provoke the failure:

void AnotherThrowingMethod() {
  throw new RuntimeException();
}
void ThrowingMethod() {
  try {
    AnotherThrowingMethod();
  }
  catch(RuntimeException ex) {
    throw new RuntimeException(ex);
  }
}
void foo() throws Exception {
  Exception cachedException = null;
  try {
    ThrowingMethod();
  }
  catch(Exception ex) {
    cachedException = ex;
    throw ex;
  }
  finally {
    try {
      ThrowingMethod();
    }
    catch(Exception ex) {
      if(cachedException != null) {
        ex.initCause(cachedException);
        // error: IllegalStateException 
        // Exception ex already has a cause
      }
      throw ex;
    }
  }
}

The idiomatic approach could be written to check for this situation, for example the handling of the PreparedStatement could become:

if(ps != null) {
  try {
    ps.close();
  }
  catch(SQLException ex) {
    if(ex.getCause() == null) {
      if(cachedException != null) {
        ex.initCause(cachedException);
      }
    }
    throw ex;
  }
}

But this will mean that the original exception is lost, as discussed above, making Hagger's approach better in this case.

Handling exceptions thrown while in a finally block is problematic in the context of an existing exception. This article has presented some approaches that solve at least some of the problems discovered in the example but no approach is entirely satisfactory. For the example presented the idiomatic solution works and is the best solution.

In the wider context of a general solution each approach has drawbacks or will not work, for example the idiomatic approach will fail if the exception being handled already has a cause. Of the approaches presented I would use, in order of preference, the idiomatic version, then Hagger's approach (if the exceptions being handled could already have a cause). I would resist using the approach in [Warren-] as ignoring exceptions is a particularly bad idiom and should be avoided under any circumstances.