3
\$\begingroup\$

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.

\$\endgroup\$
  • \$\begingroup\$ if (LOG.isDebugEnabled()) { and if (Log4jUtils.isDebugLevel(LOG)) { looks like they checked the same thing, but they are not the same, is there a difference ? Do you really need to check if debug is set ? (I may be wrong, I'm just curious) \$\endgroup\$ – Marc-Andre Mar 4 '14 at 13:56
  • \$\begingroup\$ @Marc-Andre: yes, you are right. This is pasted from different sections of our code. For some reason, I always used to code Level.DEBUG.isGreaterOrEqual(logger.getEffectiveLevel()) to check, so I made a wrapper method so we didn't have to litter our code with that every time. I'm not sure if I somehow overlooked the isDebugEnabled() method, or if it was added after we wrote the earlier code. \$\endgroup\$ – Sam Goldberg Mar 4 '14 at 20:02
3
\$\begingroup\$

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;

    }
}
\$\endgroup\$
  • \$\begingroup\$ How about changing the helper method to return a collection that would contain the data from the resultset and close all resultset, statement and connection in the finally section? I believe one should not keep the resultset(or any JDBC connection related referenced) hanging around for data processing. Just my opinion. \$\endgroup\$ – Srini Kandula Mar 4 '14 at 16:31
  • \$\begingroup\$ @SriniKandula The resultset, and all related references are closed. Nothing escapes the execute() method. The work is done inside the context of the ResultSet, and the return value, in this case, is the number of rows processed. \$\endgroup\$ – rolfl Mar 4 '14 at 16:33
  • \$\begingroup\$ my comment refers to the original post and I'm asking your opinion about my suggestion. However in your execute() method also the resultset is open while widget.doSomethingWith() is running. My point is one shouldn't keep the connection alive while we process the data. \$\endgroup\$ – Srini Kandula Mar 4 '14 at 16:42
  • 1
    \$\begingroup\$ @SriniKandula for the most part, converting a ResultSet to a Collection of some sort is a waste of time and effort, and you lose the ability to do native type management (getInt(col), getString(...), etc.). While you should not keep a ResultSet open for longer than necessary, I would consider any code that uses the ResultSet without sleeping, blocking, or user-interaction to be fine. \$\endgroup\$ – rolfl Mar 4 '14 at 16:55
  • 1
    \$\begingroup\$ Thanks for the ideas above. We have a standalone application, so we would need to integrate one of the libraries you mentioned. One question I have about the code you posted: ResultSet rs = con.executeQuery("..."), which type of Connection object has an executeQuery() method? This would be good to use, because it avoids the whole issue of creating a separate Statement object which we rarely care about. (Some times we reuse PreparedStatement objects, but it is very rare.) \$\endgroup\$ – Sam Goldberg Mar 4 '14 at 20:21

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service, privacy policy and cookie policy

Not the answer you're looking for? Browse other questions tagged or ask your own question.