Java closing connections and findbugs

Question

In our code we usually use the following pattern:

Connection conn;
try{
    conn = getConnection();
    //Do databasey stuff
}catch(Exceptions that get thrown){
}finally{
    try{
        conn.close();
    }catch(SQLException ex){
        logger.error("Failed to cleanup database connection",ex);
    }
}

However findbugs doesn't like this. Since conn.close() can throw an exception then the connection isn't guaranteed to be closed. Is findbugs being too pedantic or is there a better way to close database connections.

Edit: Added removed try catch around close.

Answer 1

What you really want to be doing is to combine "The Elite Gentleman"'s answer with the @edu.umd.cs.findbugs.annotations.SuppressWarnings( "OBL_UNSATISFIED_OBLIGATION" ) annotation. FindBugs seems to only be happy if you the complete closing in method in the following manner (which is btw the preferred sequence for doing so):

...
}finally{
    try{ 
       resultSet.close();
    }catch( SqlException e ){
       //log error
    }finally{
       try{
          statement.close();
       }catch( SqlException e ){
          //log error
       }finally{
          try{
              connection.close();
          }catch( SqlException e ){
              //log error
          }
       }
    }
}

Which is very verbose and you probably don't want to do if for no other reason than the love of your carpal tunnel, thus you should use the DBUtils.closeQuietly() method (or create your own, your call). However, FindBugs doesn't recognise this (i.e. using the library or your own method) as properly closing the resources and issues you a warning. In this case it is clearly a false positive. Therefore must make sure its the only warning you are getting and then disable that specific warning for that method.

@edu.umd.cs.findbugs.annotations.SuppressWarnings( "OBL_UNSATISFIED_OBLIGATION" )
public void doStuff( final Connection connection ){
    try{
        //Do databasey stuff
    }catch( SqlException e ){
        //throw a glorious exception....
    }finally{
        DbUtils.closeQuietly( resultSet  );
        DbUtils.closeQuietly( statement  );
        DbUtils.closeQuietly( connection );
}

This way you clean up your resources with the few lines of code and avoid a FindBugs warning.

Answer 2

There's already a utility that does what @duffymo mentioned: DbUtils from Apache.

• DbUtils.close(ResultSet);
• DbUtils.close(Statement);
• DbUtils.close(Connection);

The APIDocs shows all available methods.

---

Update

Here's an example:

import org.apache.commons.dbutils;


Connection conn;
try{
    conn = getConnection();
    //Do databasey stuff
} catch(Exception e){
    //throw a glorious exception....
} finally{
    DbUtils.closeQuietly(conn); //This hides the SQLException thrown by conn.close();
    //or 
    //DbUtils.close(conn);
}

---

Update: As suggested by ArtB, if you're finally closing resources and connections and findBugs is being a nagger, you can add the following annotation (on top of the method).

@edu.umd.cs.findbugs.annotations.SuppressWarnings("OBL_UNSATISFIED_OBLIGATION")

Answer 3

Yes, there's a better way.

Create a static method that wraps the close in a try/catch:

public class DatabaseUtils
{
    public static void close(Connection c)
    {
        try
        {
            if (c != null)
            {
                c.close();
            }
        }
        catch (SQLException e)
        {
            // print or log stack trace
        }
    }

    // same for Statement and ResultSet
}

Answer 4

Yes, you should encapsulate your close in a try block, but there is a cleverer way.

try {
    Connection c = getConnection();
    try {
        //do stuff
    } finally {
        c.close();
    }
} catch (SQLException e) {
    //Catch exceptions
}

Answer 5

There isn't really a better way, but if you want to be sure that you catch everything, modify your pattern to this:

Connection conn;
try{
    conn = getConnection();
    //Do databasey stuff
}catch(Exceptions that get thrown){
}finally{
    try {
       conn.close();
    }
    catch  (SQLException se) {
       log.error("database problems...");
       // do more stuff if you need to
    }
}

Answer 6

You can avoid the whole thing by using something like Spring JDBCTemplate, which properly encapsulates all the open/close logic for you, simplifies your code, makes it cleaner and easier to read (as well as more likely to be correct).

An example, reading some columns from a table into a list of key-value pairs (yeah, ugly, but easy to understand):

        List<Map<String, Object>> resultList = jdbcTemplate.query(query,
                new RowMapper<Map<String, Object>>() {
                    @Override
                    public Map<String, Object> mapRow(ResultSet rs,
                            int rownum) throws SQLException {
                        Map<String, Object> row = new HashMap<String, Object>();
                        int colIndex = 0;
                        row.put(CONTENTID_KEY, rs.getInt(++colIndex));
                        row.put(CONTENTTYPEID_KEY, rs.getInt(++colIndex));
                        row.put(DESCRIPTION_KEY, rs.getString(++colIndex));
                        row.put(CODE_KEY, rs.getString(++colIndex));
                        return row;
                    }
                });

For exception handling, see spring jdbcTemplate how to catch exception?