Design considerations for classes/methods using JDBC to ensure resources closed correctly

Question

In reviewing code in our application, I see a tension between what makes a convenient design for methods accessing database data versus coding to ensure that the JDBC resource objects are closed properly.

An example problem:

// Application.java: Singleton class for objects which want to execute SQL queries

/**
 * Execute a query and return the Result. Provide a common point of
 * connection/statement handling
 * 
 * @param sql
 *            SQL to execute
 * @return a ResultSet
 */
public ResultSet executeQuery(String sql) throws Exception {
    DBConnectionObj connectionObj = dbPoolMgr.getConnection();
    try {
        if (LOG.isDebugEnabled()) {
            LOG.debug("StateServer.executeQuery(): executing query=" + sql);
        }
        return connectionObj.executeQuery(sql);
    } catch (Exception e) {
        LOG.error("executeQuery(): error executing SQL=" + sql);
        throw e;
    } finally {
        dbPoolMgr.putConnection(connectionObj);
    }
}


// DBConnectionObj.java: JDBC Connection wrapper used in a connection pool

public ResultSet executeQuery(String sql, Connection connection) {
  try {
    if (Log4jUtils.isDebugLevel(LOG)) {
            LOG.debug("executeQuery(): " + sql);
        }
        return connection.createStatement().executeQuery(sql);
  } catch (SQLException e) {
        UnsuccessfulSQLExecutionResult result = new UnsuccessfulSQLExecutionResult(e, sql);
        LOG.error("executeQuery(): exception executing query. SQL info=" +        result.getContents());
        e.printStackTrace();
        return null;
  }         
}

With above helper functions, other application classes can just pass the specific SQL and get back a reference to the ResultSet:

    ResultSet rs = Application.getInstance().executeQuery(getSql());
    while (rs.next()) {
            // process data
    }
    rs.close();

The problem with the DBConnectionObj.executeQuery() method, is that it creates a temporary Statement to execute the query, and this temporary object is never closed. But also, because the method returns the ResultSet to the caller, it can't close the Statement until the caller is ready to close the ResultSet. (The code doesn't need to close the Connection object, because it stays open for the lifetime of the application, and is part of a pool of open database connections.)

One solution would be to make sure every caller to the executeQuery() method, always does this:

rs.close();
rs.getStatement().close();

So the question is this: for usefulness, having a method which takes a SQL string for input and returns a ResultSet feels like a good design. However, it imposes requirement that callers make sure to close both the ResultSet and the Statement, and it can't take advantage of the safer Java 7 try-close feature:

try(ResultSet rs = Application.getInstance().executeQuery(getSql())) {
    while (rs.next()) {
        // process data
    }
}  // rs is automatically closed, but not the Statement object

Any comments or suggestions on a better design are appreciated.

Answer 1

JDBC has always been a PITA when it comes to handling/closing resources.

One of the big advancements in the past while has been the advent of ConnectionPools and abstraction layers.... If you are inside a framwork like tomcat, or WebSphere, these are built in. Otherwise you can use things like Apache DBCP or C3PO. For both of these frameworks, when you close the Connections it closes any created Statements.

Using a pool like this allows you to trust in the pooling layer... but, I would still recommend that you use a 'clean' implementation for your code. What I have used in the past, and I think is the neatest method, is to have an abstract class like:

public abstract class DBCallable<T> implements Callable<T> {
    private final ConnectionPool pool;

    public DBCallable(ConnectionPool pool) {
        this.pool = pool;
    }

    public <T> call() throws SQLException {
        try (Connection con = pool.getConnection()) {
            return execute(con);
        }
    }

    protected abstract T execute(Connection con);
}

Then, you can do all sorts of things....

For example, you need to populate some widget:

final Widget widget = new Widget();
final ConnectionPool pool = .......

new DBCallable<Integer>(pool) {
    Integer execute(Connection con) {
        try (Statement stmt = con.createStatement()) {
            int count = 0;
            try (ResultSet rs = con.executeQuery("...")) {
                while (rs.next()) {
                    count++;
                    widget.doSomethingWith(rs.get(1));
                }
            }
            return count;
        }
    }
}

You can even supply one of those to an ExecutorService, and get a Future from it, and then then run these things in alternate threads.

Very versatile.

Edit: If you would prefer to have (easy) access to the Statement instead of the Connection, create an abstract sub-class of the DBCallable:

public abstract class StmtCallable<T> extends DBCallable<T> {

    public DBCallable(ConnectionPool pool) {
        super(pool);
    }

    protected T execute(Connection con) {
        try (Statement stmt = con.createStatement()) {
            return query(stmt);
        }
    }

    protected abstract T query(Statement stmt);
}

Then, if you want a clean statement each time for your code, you can have

new StmtCallable<Integer>(pool) {
    Integer query(Statement stmt) {
        int count = 0;
        try (ResultSet rs = stmt.executeQuery("...")) {
            while (rs.next()) {
                count++;
                widget.doSomethingWith(rs.get(1));
            }
        }
        return count;

    }
}