Bug 50571 - Tomcat 7 JDBC connection pool exception enhancement
Summary: Tomcat 7 JDBC connection pool exception enhancement
Status: NEEDINFO
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
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2011-01-11 12:29 UTC by Jeremy Norris
Modified: 2011-01-12 12:37 UTC (History)
0 users



Attachments
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
Details

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.

Thoughts?
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

best
Filip
Comment 2 Jeremy Norris 2011-01-11 18:36:33 UTC
Hello,

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...

Thanks.
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);
            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.

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

best
Filip
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;
        }
        handler=handler.getNext();
    }
    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.

Thoughts?
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.