Summary: | Add a validationQueryTimeout property | ||
---|---|---|---|
Product: | Tomcat Modules | Reporter: | Daniel Mikusa <dmikusa> |
Component: | jdbc-pool | Assignee: | Tomcat Developers Mailing List <dev> |
Status: | RESOLVED FIXED | ||
Severity: | enhancement | CC: | donnchadh |
Priority: | P2 | ||
Version: | unspecified | ||
Target Milestone: | --- | ||
Hardware: | PC | ||
OS: | All | ||
Attachments: |
First attempt at a patch.
Second patch Third patch |
Description
Daniel Mikusa
2013-03-13 20:04:20 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.
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. 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 Created attachment 30086 [details]
Second patch
Added patch with additional testcase.
Sorry, found an issue with my second patch. Getting an NPE with testOnConnect set to true. Need to revisit and see what is happening. 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.
Any idea when this will be released ? Thanks Rob Any idea when this will be released ? Thanks Rob (In reply to Rob Dunn from comment #8) > Any idea when this will be released ? Not likely until it's fixed. Thanks for the report. Fixed in trunk and 7.0.x and will be included in 7.0.43 onwards. |