Bug 54693 - Add a validationQueryTimeout property
Summary: Add a validationQueryTimeout property
Status: RESOLVED FIXED
Alias: None
Product: Tomcat Modules
Classification: Unclassified
Component: jdbc-pool (show other bugs)
Version: unspecified
Hardware: PC All
: P2 enhancement with 5 votes (vote)
Target Milestone: ---
Assignee: Tomcat Developers Mailing List
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2013-03-13 20:04 UTC by Daniel Mikusa
Modified: 2013-09-09 10:21 UTC (History)
1 user (show)



Attachments
First attempt at a patch. (12.39 KB, patch)
2013-03-13 20:11 UTC, Daniel Mikusa
Details | Diff
Second patch (13.42 KB, patch)
2013-03-20 19:40 UTC, Daniel Mikusa
Details | Diff
Third patch (17.77 KB, patch)
2013-03-22 02:02 UTC, Daniel Mikusa
Details | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Daniel Mikusa 2013-03-13 20:04:20 UTC
While it's possible to set the query timeout using the QueryTimeoutInterceptor, this will set the same timeout for all queries.  It would be nice to be able to set an independent timeout for the validation query.

In addition, DBCP supports this feature [1], so it would be nice to have this feature for compatibility / migration purposes.

[1] - https://issues.apache.org/jira/browse/DBCP-226

Thanks
Comment 1 Daniel Mikusa 2013-03-13 20:11:10 UTC
Created attachment 30045 [details]
First attempt at a patch.

Attaching my first attempt at a patch.  This adds a "validationQueryTimeout" property, which calls "setQueryTimeout" on the validation query's statement.  Includes unittest and documentation update.
Comment 2 Christopher Schultz 2013-03-15 18:59:18 UTC
Only one comment: since a value of "0" disabled query timeout (it will wait forever), why not make the default 0 instead of -1?

Also, you mention that QueryTimeoutInterceptor will modify the query timeouts of all queries... how does it interact with this patch? Will QueryTimeoutInterceptor set the query timeout and then your code re-sets the query timeout (presumably to a different value)? If that's the case, what happens when validationQueryTimeout=0 and you are using the QueryTimeoutInterceptor? A reasonable user might expect that the validation query would /never/ time out, but the QueryTimeoutInterceptor would affect it.
Comment 3 Daniel Mikusa 2013-03-20 19:33:48 UTC
Comments inline below...

>Only one comment: since a value of "0" disabled query timeout (it will wait forever), why not make the default 0 instead of -1?

I implemented it the same way as DBCP to try and make it as compatible as possible.  

In my implementation (same as DBCP), the value must be greater than 0 or the connection pool code won't call "setQueryTimeout", which means that by default the connection pool won't call "setQueryTimeout".  

I like this approach because it won't change anything by default.  In other words, the query timeout value would default to what is specified by the JDBC driver or perhaps by a driver specific setting.

I'm open to other suggestions though.


>Also, you mention that QueryTimeoutInterceptor will modify the query timeouts of all queries... how does it interact with this patch?

Sorry, I was under the impression based on some comments I read on a previous bug that the QueryTimeoutInterceptor would set the query timeout for a validation query.  I tested this a bit and it turns out to not be true.  In my tests, I saw that the validation query is executed against the underlying Connection, not the ProxyConnection.  Because it runs against the underlying Connection, this means that the interceptors are not executed against it.


>Will QueryTimeoutInterceptor set the query timeout and then your code re-sets the query timeout (presumably to a different value)? If that's the case, what happens when validationQueryTimeout=0 and you are using the QueryTimeoutInterceptor? A reasonable user might expect that the validation query would /never/ time out, but the QueryTimeoutInterceptor would affect it.

As I mentioned above, the QueryTimeoutInterceptor and validationQueryTimeout should not affect each other.  Just to be sure, I added a test case to check this.  Attaching new patch with that test case added.

Dan
Comment 4 Daniel Mikusa 2013-03-20 19:40:42 UTC
Created attachment 30086 [details]
Second patch

Added patch with additional testcase.
Comment 5 Daniel Mikusa 2013-03-20 20:55:39 UTC
Sorry, found an issue with my second patch.  Getting an NPE with testOnConnect set to true.  Need to revisit and see what is happening.
Comment 6 Daniel Mikusa 2013-03-22 02:02:03 UTC
Created attachment 30094 [details]
Third patch

Ok, so there was an issue with my second patch.  When "testOnConnect" was enabled, the validation query specified times out and a connection was requested, an NPE would be thrown from "setupConnection", where "con.getHandler()" is called.

    protected Connection setupConnection(PooledConnection con) throws SQLException {
        //fetch previously cached interceptor proxy - one per connection
        JdbcInterceptor handler = con.getHandler();
        if (handler==null) {

This was because "borrowConnection", at the point below, returns null.

    public Connection getConnection() throws SQLException {
        //check out a connection
        PooledConnection con = borrowConnection(-1,null,null);
        return setupConnection(con);
    }

It returned null because of the following conditions...

  - there are no available connections
  - "borrowConnection(..)" tries to create one by calling "createConnection(..)"
  - because "testOnConnect" is true, "createConnection(..)" will call "con.validate(..)" and the validation query will run
  - because we use a slow query and set a low validation query timeout, the validation query is interrupted
  - "con.validate(..)" returns false
  - this causes "createConnection(..)" to clean up and return null
  - "borrowConnection(..)" returns what was returned by "createConnection(..)", which is null
  - the null then slips through to "setupConnection(con)" and causes the NPE.

I fixed this by throwing an SQLException in "createConnection(..)" when "con.validate(..)" returns false.  This causes "createConnection(..)" to clean up and then re-throw the SQLException.  This exception bubbles up to the user's code alerting them that the pool cannot create any new connections because the validation query fails.

A very similar situation happens if the validation query is invalid, such as "SELECT".  This fixes both conditions.  Unit tests included in the latest patch.
Comment 7 Rob Dunn 2013-06-25 05:26:57 UTC
Any idea when this will be released ?

Thanks
Rob
Comment 8 Rob Dunn 2013-06-25 06:23:22 UTC
Any idea when this will be released ?

Thanks
Rob
Comment 9 Christopher Schultz 2013-06-25 12:12:01 UTC
(In reply to Rob Dunn from comment #8)
> Any idea when this will be released ?

Not likely until it's fixed.
Comment 10 Keiichi Fujino 2013-09-09 10:21:48 UTC
Thanks for the report.
Fixed in trunk and 7.0.x and will be included in 7.0.43 onwards.