Is there an issue with closing our database connections in the "Finally" block of a Try statement?

Question

I'm doing some refactoring for our application, and trying to reduce the number of issues reported in our ISO Scan (a static code analysis tool based on HP Fortify). Right now, what I'm trying to address is the "Unreleased Resource: Database" issues our application has built up. One of the most prominent reasons for this is constructs like this:

Connection conn = null;
Boolean myConn = false;
try{
   if(conn == null){
     conn = DatabaseUtil.getConnection();
     myConn = true;
   }
   result = DbClass.getObject(conn, otherParameters);
}catch(DatabaseException de){
  throw de;
}catch(SQLException sqle){
  throw new DatabaseException("Error Message");
}finally{
  if(myConn && conn != null){
    try{
      conn.rollback();
      conn.close();
    }catch(SQLException sqle){}
}

This is a fairly standard construct in our application to ensure the database connection is closed at the end of our Try block. Yet every time we use it, the tool reports it as an issue.

Is there anything incorrect about this construct, and is there anything we can do to fix it if it is an issue?

Answer 1

And the tools are right!

Apart from the (possible) limitations of the analysis - it may or may not understand that (conn != null) == myConn, observe that rollback throws an SqlException. If it does, then close is never called, which is precisely the definition of a resource leak. Now, it may be the case that with your current DB, drivers and pooling libraries if rollback throws, then the connection is already released, but this is not true in the general case, hence a potential problem.

As noted in comments you can simplify much of this construct by using try-with-resources if using Java 7. It also becomes much simpler to reason about and include a rollback via a custom class that does rollback in its close() method in a try block:

@Override
public void close() throws SQLException {
    try {
        rollback();
    catch (...) {
        //Do stuff
    finally {
        super.close();
    }
}

Here you are 100% guaranteed that close() will actually run, whatever rollback does.

If working with legacy software, the above code can be directly put into the try:

Connection conn = null;
try {
   conn = DatabaseUtil.getConnection();
   result = DbClass.getObject(conn, otherParameters);
} catch(SQLException sqle) {
  throw new DatabaseException("Error Message");
} finally {
  if (conn != null) {
    try{
        conn.rollback();
    } catch(SQLException sqle) {
        //try-with-resources would add this to suppressed exceptions, you can either log or rethrow
    } finally {
        conn.close();
    }
}

Answer 2

Because your release of resources is conditional on the state of a boolean variable and encased in another try block, the static analyzer must be deciding that rollback() and close() are not guaranteed to execute.

Check the documentation for the Connection object of the type returned by the getConnection() factory method, and see if the methods rollback() and close() will even throw an exception. If they won't, you don't need that additional try catch block when calling those methods.

But first, eliminate the boolean variable that you're using to signal an open connection. You shouldn't need it; you shouldn't get a connection object back from the getConnection() factory method unless it was successful at establishing a connection.

Note that Fortify's security report suggests the following code structure:

public void execCxnSql(Connection conn) {
    Statement stmt;
    try {
        stmt = conn.createStatement();
        ResultSet rs = stmt.executeQuery(CXN_SQL);
        ...
    } 
    finally {
        if (stmt != null) {
            safeClose(stmt);
        }
    }
}
public static void safeClose(Statement stmt) {
    if (stmt != null) {
        try {
            stmt.close();
        } catch (SQLException e) {
            log(e);
        }
    }
}

...which suggests that the boolean variable is the problem, not the additional try block.

Answer 3

Eliminate the condition, since the compiler things the code could not run at all.

Then catch Exception instead of SQLException so it catches NullPointerException in case the connections are not initialized. I usually set the object to null after closing it, because a recommendation I read in a Sybase's EAServer paper and, maybe, it has nothing to do with Java's garbage collector but with EAServer's connection pool, and maybe I'm "cargo-culting" here.

Connection conn = null;
Boolean myConn = false;
try{
   if(conn == null){
     conn = DatabaseUtil.getConnection();
     // myConn = true; no need for this
   }
   result = DbClass.getObject(conn, otherParameters);
}catch(DatabaseException de){
  throw de;
}catch(SQLException sqle){
  throw new DatabaseException("Error Message");
}finally{
    try{
      conn.rollback();
      conn.close();
      conn=null; // good practice according to people at Sybase
    }catch(Exception e){
         //fails silently when conn is not initialized.
    }
}