Bug 50571 - Tomcat 7 JDBC connection pool exception enhancement
Summary: Tomcat 7 JDBC connection pool exception enhancement
Alias: None
Product: Tomcat Modules
Classification: Unclassified
Component: jdbc-pool (show other bugs)
Version: unspecified
Hardware: PC All
: P2 normal (vote)
Target Milestone: ---
Assignee: Tomcat Developers Mailing List
Depends on:
Reported: 2011-01-11 12:29 UTC by Jeremy Norris
Modified: 2020-05-13 03:56 UTC (History)
0 users

Created against http://svn.apache.org/repos/asf/tomcat/trunk on 20110111 (6.41 KB, application/octet-stream)
2011-01-11 12:29 UTC, Jeremy Norris

Note You need to log in before you can comment on or make changes to this bug.
Description Jeremy Norris 2011-01-11 12:29:56 UTC
Created attachment 26476 [details]
Created against http://svn.apache.org/repos/asf/tomcat/trunk on 20110111

I am working with the new JDBC connection pool in Tomcat 7, and it's great!  Nice work.

Once enhancement I would like to suggest is more control over the SQLException's that come out of the DataSource.  Certain interceptors (eg: ConnectionState) catch SQLException's and log them, thereby preventing the layer above DataSource from being able to detect and handle SQL problems.

For example, if a DataSource.getConnection() is created with an invalid database name, the ConnectionState interceptor will catch and log (eg: reset()), leaving the caller with no control over log output or knowing what the specific problem with the DataSource was.  I am working on an application where the datasources are more dynamic than the typical statically defined datasources in managed application servers, therefore I want to be able to detect and handle SQLException's that are raised from the ConnectionPool.

I have provided a patch that illustrates the type of change I'm talking about.  As you can see, this causes SQLExceptions to propagate through several other APIs, but I think this is an improvement.  This different exception handling strategy could also be enabled with a property.

Comment 1 Filip Hanik 2011-01-11 16:47:08 UTC
hi Jeremy,
I'm looking at your patch, and I will take a look at the connection state interceptor. most SQL exceptions, as you say, should percolate through when it makes sense.

I wont apply the patch as it is, as I don't understand why most of the methods just change the signature of the method by adding "throws SQLException".

for example ConnectionPool.abandon doesn't really need to throw a SQLException

Comment 2 Jeremy Norris 2011-01-11 18:36:33 UTC

This initial use-case I was needing was for SQLExceptions originating in ConnectionState.reset() to propagate out of ConnectionPool.getConnection().

The rest of the changes cascaded from this:  eg: ConnectionPool.abandon() -> ConnectionPool.release() -> PooledConnection.setHandler() -> JdbcInterceptor.reset().

I suspect there will be more and that most APIs will possibly throw SQLException...

Comment 3 Filip Hanik 2011-01-11 19:02:44 UTC
I see, exceptions can't simply be propagated like that, since that would cancel out events/code that needs to run.

    protected void finalize(PooledConnection con) {
        JdbcInterceptor handler = con.getHandler();
        while (handler!=null) {
            handler.reset(null, null);

If a reset() throws an exception, the rest of the interceptors wouldn't be notified here, and the exception would not mean very much.

So, probably better to address case by case, and see what needs to be done

Comment 4 Jeremy Norris 2011-01-11 20:52:34 UTC
(In reply to comment #3)
>     protected void finalize(PooledConnection con) {
>         JdbcInterceptor handler = con.getHandler();
>         while (handler!=null) {
>             handler.reset(null, null);
>             handler=handler.getNext();
>         }
>     }
> If a reset() throws an exception, the rest of the interceptors wouldn't be
> notified here, and the exception would not mean very much.

Currently, if a connection pool is configured for a database that does not exist, the first place a SQLException occurs is reset() (with the following call stack for mysql):

Caused by: com.mysql.jdbc.exceptions.jdbc4.MySQLSyntaxErrorException: Unknown database 'db_name'
	at sun.reflect.NativeConstructorAccessorImpl.newInstance0(Native Method)
	at sun.reflect.NativeConstructorAccessorImpl.newInstance(NativeConstructorAccessorImpl.java:39)
	at sun.reflect.DelegatingConstructorAccessorImpl.newInstance(DelegatingConstructorAccessorImpl.java:27)
	at java.lang.reflect.Constructor.newInstance(Constructor.java:513)
	at com.mysql.jdbc.Util.handleNewInstance(Util.java:407)
	at com.mysql.jdbc.Util.getInstance(Util.java:382)
	at com.mysql.jdbc.SQLError.createSQLException(SQLError.java:1052)
	at com.mysql.jdbc.MysqlIO.checkErrorPacket(MysqlIO.java:3593)
	at com.mysql.jdbc.MysqlIO.checkErrorPacket(MysqlIO.java:3525)
	at com.mysql.jdbc.MysqlIO.sendCommand(MysqlIO.java:1986)
	at com.mysql.jdbc.MysqlIO.sqlQueryDirect(MysqlIO.java:2140)
	at com.mysql.jdbc.ConnectionImpl.execSQL(ConnectionImpl.java:2620)
	at com.mysql.jdbc.ConnectionImpl.setCatalog(ConnectionImpl.java:5116)
	at org.apache.tomcat.jdbc.pool.interceptor.ConnectionState.reset(ConnectionState.java:95)
	at org.apache.tomcat.jdbc.pool.ConnectionPool.setupConnection(ConnectionPool.java:280)
	at org.apache.tomcat.jdbc.pool.ConnectionPool.getConnection(ConnectionPool.java:169)
	at org.apache.tomcat.jdbc.pool.DataSourceProxy.getConnection(DataSourceProxy.java:111)

If reset() swallows and logs this and we don't propagate it, then the upper layer has no way of acting on this (or controlling the log output).

We can try finishing executing the interceptor stack, ie:

protected void finalize(PooledConnection con) {
    JdbcInterceptor handler = con.getHandler();
    Exception exception = null;
    while (handler!=null) {
        try {
            handler.reset(null, null);
        } catch (SQLException e) {
            exception = e;
    if (exception != null) {
        throw exception

but, if more exceptions are thrown from additional interceptors, what exception should ultimately be thrown from finalize()?

One solution is to NOT continue processing the interceptor stack.  If interceptors need to be notified, we could use another event.

Comment 5 Jeremy Norris 2011-01-11 21:18:57 UTC
Sorry, the example code above should be for ConnectionPool.setupConnection().  The problem is the same though; do we continue invoking the interceptors on an exception or not.
Comment 6 Filip Hanik 2011-01-12 11:11:22 UTC
What I propose, is to fix the actual bug, of ConnectionState swallowing the exception on pool startup.
And we fix this bug, without introducing new bugs by changing too many signatures.
Comment 7 Jeremy Norris 2011-01-12 12:37:33 UTC
Agreed, the actual bug is ConnectionState swallowing the exception on pool startup, however I suspect that fixing it properly will require some of the signatures to change.

In my opinion, SQLException's needs to propagate all the way up to ConnectionPool.getConnection().  SQLException is a checked exception so when we propagate it from JdbcInterceptor, it will affect the callers signature unless that caller swallows it (not what we want) or wraps it in a RuntimeException (a bit clumsy).

Thanks for your time and discussion, it's very appreciated.
Comment 8 bill_t_wang 2020-05-13 03:56:03 UTC
I resolved this bug in SlowQueryReport,using the second proposal by Felix Schumacher -- "extract the information to sort on from the QueryStats and sort on that information".

1, add a constructor in the inner class QueryStats for QueryStats clone
         * Create a new QueryStats object based on another QueryStats object.
         * For resolving Bug 58489, https://bz.apache.org/bugzilla/show_bug.cgi?id=58489
         * @param qs
        QueryStats(QueryStats qs){
            this.query = qs.query;
            this.nrOfInvocations = qs.nrOfInvocations;
            this.maxInvocationTime = qs.maxInvocationTime;
            this.maxInvocationDate = qs.maxInvocationDate;
            this.minInvocationTime = qs.minInvocationTime;
            this.minInvocationDate = qs.minInvocationDate;
            this.totalInvocationTime = qs.totalInvocationTime;
            this.failures = qs.failures;
            this.prepareCount = qs.prepareCount;
            this.prepareTime = qs.prepareTime;
            this.lastInvocation = qs.lastInvocation;

2, modify method removeOldest
    protected void removeOldest(ConcurrentHashMap<String,QueryStats> queries) {
        ArrayList<QueryStats> list = new ArrayList<>(queries.values());
        //For resolving Bug 58489, https://bz.apache.org/bugzilla/show_bug.cgi?id=58489
        //make a list of cloned QueryStats objects
        for(int i=0; i<list.size(); i++){
            list.set(i, new QueryStats(list.get(i)));
        Collections.sort(list, queryStatsComparator);
        int removeIndex = 0;
        while (queries.size() > maxQueries) {
            String sql = list.get(removeIndex).getQuery();
            if (log.isDebugEnabled()) log.debug("Removing slow query, capacity reached:"+sql);