Tomcat JDBC connection pool issue: "Statement is closed"

Question

I have a server application that uses the Tomcat JDBC connection pool.

This is the code I use to create the DataSource:

PoolProperties connProperties = new PoolProperties();
connProperties.setUrl(resources.getProperty("db.url"));
connProperties.setDriverClassName(resources.getProperty("db.driver"));
connProperties.setUsername(resources.getProperty("db.user"));
connProperties.setPassword(resources.getProperty("db.password"));
connProperties.setJmxEnabled(true);
connProperties.setTestWhileIdle(false);
connProperties.setValidationQuery("SELECT 1");
connProperties.setTestOnReturn(false);
connProperties.setValidationInterval(30000);
connProperties.setTimeBetweenEvictionRunsMillis(30000);
connProperties.setMaxActive(500);
connProperties.setInitialSize(50);
connProperties.setMaxWait(10000);
connProperties.setRemoveAbandonedTimeout(60);
connProperties.setMinEvictableIdleTimeMillis(60000);
connProperties.setSuspectTimeout(60);
connProperties.setMaxIdle(50);
connProperties.setMinIdle(10);
connProperties.setLogAbandoned(false);
connProperties.setRemoveAbandoned(true);
connProperties.setJdbcInterceptors("org.apache.tomcat.jdbc.pool.interceptor.ConnectionState;"+
"org.apache.tomcat.jdbc.pool.interceptor.StatementFinalizer");

dataSource = new DataSource();
dataSource.setPoolProperties(connProperties);

Then I have a method to get a connection from the pool

protected Connection getDbConnection() throws Exception
{
    dbConn = dataSource.getConnection();
    return dbConn;
}

And every time I want to execute a statement I call this code:

protected CallableStatement executeCSqlQuery(String sql) throws Exception
{
    CallableStatement cstmt;
    ResultSet rs = null;

    try {
        cstmt = getDbConnection().prepareCall(sql);     
        cstmt.execute();            
    } catch (SQLException e) {
        throw e;
    }

    return cstmt;
}

And this is an example of a call to the previous code:

try {
    cstmt = dbConnection.executeCSqlQuery(query);
    rs = cstmt.getResultSet();
} catch (Exception e) {
    // do smething
} finally {
    try {
        if (cstmt != null) {
            cstmt.close();
        }
        dbConnection.shutdown();
    } catch (Exception e) {
        // do something
    }
}

public void shutdown() {
    if (this.dbConn != null) 
        this.dbConn.close();
}

The problem I'm facing is that every now and then, I'm getting an exception "Statement is closed" when I execute a call in a Thread every X seconds. I'm not sure why this happens. I'm thinking that it could be a driver bug or something failing with the connection to the database (that runs in a different server).

I'm out of ideas. What am I missing?

Should I use c3p0 connection pool instead?

Sorry, forgot that one: if (this.dbConn != null) this.dbConn.close(); — Reznik, Oct 23 '13 at 14:06
In your last block of code I don't think you should be calling shutdown(). The connection pool might try and use it again. The connection pool will shutdown() a connection when it feels necessary. — Peter Quiring, Oct 28 '13 at 15:17
So you mean i should only close the statement and leave the connection open? What i read here is the opposite: http://stackoverflow.com/questions/4938517/closing-jdbc-connections-in-pool — Reznik, Oct 28 '13 at 16:57
You definitely want to call `connection.close()` because this is how you mark that it's no longer in use and it can be recycled. As it's bean explained in the link above, the framework will then decide whether to close it or return it to the pool according to its settings. — Morfic, Nov 08 '13 at 13:20
As a side note: common practice for the web applications deployed on some Servlet container (like Tomcat) or Application Server (like GlassFish, WebLogic, WebSphere etc) is to configure JDBC connection pooling in *context.xml* file placed under *META-INF* folder. See [this example](http://stackoverflow.com/questions/13595794/jdbc-connection-pool-runs-out-of-connections-when-context-reload-true-is-enabl). And also [The Tomcat JDBC Connection Pool](http://people.apache.org/~fhanik/jdbc-pool/jdbc-pool.html) is absolutely OK to use (of course it's a matter of preference and you can choose c3p0). — informatik01, Nov 11 '13 at 01:44

score 4 · Accepted Answer · edited Nov 11 '13 at 01:43

I created the bounty to help out Reznik but I ended up figuring out what the problem was by looking at his code.

The problem is that every time a new connection is fetched from the pool in

protected Connection getDbConnection() throws Exception
{
    dbConn = dataSource.getConnection();
    return dbConn;
}

the object dbConn is updated to a new connection.

Example:

T1 invokes getDbConnection()

T2 invokes getDbConnection()

T1 executes the query, processes the resultSet and calls shutdown()

public void shutdown() {
    if (this.dbConn != null) 
        this.dbConn.close();
}

Because T2 updated the object, the connection being used by T2 will be shutdown by T1

T2 tries to use the connection but it is already closed.

This way, instead of always updating the connection, just return it and then add the extra logic to close the connection that was fetched from the pool.

Basically 'conn' should not be a member variable, otherwise it isn't thread-safe. — user207421, Nov 11 '13 at 01:36
@EJP +1, it's very important moment. Actually, every method that executes some SQL query should get **its own and NEW** Connection, Statement (or PreparedStatement) and ResultSet, i.e. they should be local variables (declared inside the method). And as soon as the main logic is done, those resources MUST be closed in the finally block. — informatik01, Nov 11 '13 at 15:55

score 2 · Answer 2 · edited Nov 11 '13 at 01:30

2

I notice two things:

1) connProperties.setTestOnReturn(false); which should be changed to true. Otherwise your connection isn't necessarily valid. And you might not get notified until you receive an invalid Statement.

2) You should ALWAYS close your ResultSet, Statement and Connection objects. It does involve a lot of boilerplate, or you could use my static Close utility method.

edited Nov 11 '13 at 01:30

informatik01

16,038
10
74
104

answered Nov 09 '13 at 21:51

Elliott Frisch

198,278
20
158
249

score 1 · Answer 3 · answered Nov 08 '13 at 13:11

1

1) protected CallableStatement executeCSqlQuery(String sql) throws Exception returns the callable statement. If you are trying to use it again after closing it inside the method, you'll probably get that error. All the processing involving that object should be done before closing it.

2) The same method has a catch(SQLException ){throw e}. You'd either want to handle the exception there, or if you want to propagate it remove the try-catch

3) I can see in the last piece of code rs = cstmt.getResultSet(); It is always a good practice to close the resources in the inverse order that they were obtained, as you've read in the post you linked for Peter. So rs.close(); cstmt.close(); connection.close();. It's not included in your sample, but if you're returning the ResultSet after closing it, you'll have the same problem described at 1)

answered Nov 08 '13 at 13:11

Morfic

15,178
3
51
61

Answering your marks: 1) when executeCSqlQuery returns, right after, is the getResultSet. There's no close() between. 3) the "statement is closed" error happens on the getResultSet(). Which means it never gets it. – Reznik Nov 08 '13 at 14:52
If you get the exception on `cstmt.getResultSet()` it means that the statement was closed by someone else while you were using it. You also said that you are running this inside a thread, so I'd suspect a multithreading synchronization issue – Morfic Nov 08 '13 at 15:06
1

Being a pool, shouldn't an open statement be used by the thread who opened it? If a 2nd thread comes along and opens another statement, it's a different connection to the pool, right? – Reznik Nov 08 '13 at 15:36
It really depends on how you store and use the objects in your code, like this field `cstmt = dbConnection.executeCSqlQuery(query);`. Can you post the full code, including the thread part? – Morfic Nov 08 '13 at 15:39
All the code executed by a thread is above. The threads execute the last block where it reads "try { cstmt = dbConnection.executeCSqlQuery(query); (...)" – Reznik Nov 08 '13 at 15:51
I understand that, but it is important to see and understand how you create your objects and how they are shared across the threads – Morfic Nov 08 '13 at 16:01

Tomcat JDBC connection pool issue: "Statement is closed"

3 Answers3

Linked

Related