public boolean validate(int validateAction) {
        return validate(validateAction,null);
    }


There should be plugaable Validator which could be configured in the poolProperties. e.g 

public boolean validate(int validateAction)
{
     ValidateAction vaction = this.poolProperties.getValidateAction();
     return vaction.validate();
}

where ValidateAction is an interface.

Some drivers e.g oracle provides ping() method on the driver which are faster than running the SQL everytime you validate the connections, so having the pluggable validator is more flexible approach.

Let me know what you think, I can provide a patch for this.

And for jdk1.6 we can have the validator which will use the jdbc specific API i.e java.sql.Connection::isValid(int timeout)

> There should be plugaable Validator which could be configured in the
> poolProperties. e.g 

why not just an interceptor if you want custom validation?

> And for jdk1.6 we can have the validator which will use the jdbc specific API
> i.e java.sql.Connection::isValid(int timeout)

eeeh, that's the title of the bug :)

(In reply to comment #2)
> > There should be plugaable Validator which could be configured in the
> > poolProperties. e.g 
> 
> why not just an interceptor if you want custom validation?

Looking over the code in ConnectionPool, it appears that the existing validation is run within borrowConnection(), when it's still possible to ask the pool for another Connection if the first one turns out to be invalid. In contrast, the JdbcInterceptors are run within setupConnection(), when it's already too late to go back to the pool. Given that, is it possible to write an interceptor that can participate in the validation process? If not, I'd agree with VickyKak that having some form of pluggable Validator would be useful.

Created attachment 25398 [details]
If no validation query is available, fall back on other approaches

This isn't exactly what was discussed, but it's an alternative solution.  I added logic so that PooledConnection.validate() falls back on other validation approaches when a query isn't available.  It first calls Connection.isClosed().  The Javadoc for that method suggests that it isn't useful for validation, but my client managed to create a situation where he had closed connections in the pool.  Next, it uses Java reflection to look for the Connection.isValid() method and calls it if it exists.  If it's not available or doesn't work (the JDBC/ODBC bridge doesn't support isValid(), for example), it falls back on calling Connection.getMetaData(), which should in theory exercise the connection.  If it doesn't, we could always go further and actually call a method on the returned DatabaseMetaData.  Let me know what you think.

I don't know the code that well so feel free to treat these comments with that in mind.

In no particular order...

It appears that this patch forces validation to happen even if the user doesn't want it. That could add a fair amount of overhead.

I'd try isValid() before trying isClosed()

The exception caught in tryIsValid() is rethrown and then silently swallowed. I can see why, but that code looks expensive. I'd probably check earlier to see if the method was available and set a flag I could check (cheaply) later.

In terms of the issues with borrowConnection() and setupConnection() would it be feasible to check if setupConnection() fails and it if does, try borrowing a new connection? I assume that if borrowConnection() fails then getConnection() should fail.

(In reply to comment #5)
Thanks Mark. Those were reasonable points. I'll respond to each.

> It appears that this patch forces validation to happen even if the user doesn't
> want it. That could add a fair amount of overhead.
One of the first things that PooledConnection.validate(int, String) does is call doValidate(validateAction). If someone hasn't turned on testOnBorrow (or whatever property is relevant to the particular validate action), the validation will be skipped.

> I'd try isValid() before trying isClosed()
I looked at some driver code and it appears that most just return the value of a boolean for isClosed(), so I was thinking I might as well ask the cheapest question first.

> The exception caught in tryIsValid() is rethrown and then silently swallowed. I
> can see why, but that code looks expensive. I'd probably check earlier to see
> if the method was available and set a flag I could check (cheaply) later.
You're right! That's a good idea. Either you're running in a Java 6 environment or you're not. It shouldn't change over time, so it should be possible to do the check only once and then store the result. I'll think about the best way to do that and then make the change.

> In terms of the issues with borrowConnection() and setupConnection() would it
> be feasible to check if setupConnection() fails and it if does, try borrowing a
> new connection? I assume that if borrowConnection() fails then getConnection()
> should fail.
That might work, but if the pool is able to determine that it's about to return a stale connection and just swap in a fresh one behind the scenes, it would make things a lot simpler for the calling code. Also, keep in mind that if the pool has multiple stale connections in it, the caller might otherwise have to call getConnection() repeatedly.

By the way, I'd be fine with a pluggable Validator approach instead. I just thought this would be simpler and generally applicable. Let me know what you think.

Created attachment 25439 [details]
Add Validator interface and references to it where appropriate

Here's an alternative which I actually prefer.  I've added a Validator interface with the following single method:
public boolean validate(Connection connection, int validateAction)

I also added get/setValidator() to PoolConfiguration and all of its implementors. When validation runs, if the configuration has a Validator, it's used in place of other forms of validation.  One thing that's nice about this approach is that it pulls the call to Connection.isValid() out of the core code.  Since I know I'll be running in a Java 6 environment, I can safely call the isValid() method directly from my Validator implementation instead of needing to use reflection to preserve Java 5 compatibility.

Don't bother with isClosed, that is a simple flag that most drivers implement to return true if and only if close() has been called. It has really nothing to do with the underlying TCP session or the database session.

Any feedback on my second patch?  I've been using it in production for more than a month now with no obvious issues.  I realize that you guys are probably busy getting Tomcat 7 ready otherwise, but if you could take a look sometime soon, that would be great.

Thanks,
Matt

(In reply to comment #9)
> Any feedback on my second patch?  I've been using it in production for more
> than a month now with no obvious issues.  I realize that you guys are probably
> busy getting Tomcat 7 ready otherwise, but if you could take a look sometime
> soon, that would be great.

One suggested change. I think you should set the validator class name rather than the validator directly so the entire config can be done in the resource. Other than that, looks good.

Created attachment 25849 [details]
Add Validator interface and allow users to configure a Validator class name

Good suggestion.  Sorry it took me so long to get to it.  I've attached a patch relative to revision 948073 that uses Validator class names rather than instances in the configuration.  Let me know if it needs any further changes before you consider it ready to apply.

Looks good to me. Patch applied. Many thanks.

Any chance of a patch for the docs?

Created attachment 25858 [details]
Add description for validatorClassName attribute to doc

Great!  I've attached a patch for the jdbc-pool.xml file, adding a description of the new validatorClassName attribute.  I had more trouble creating the patch than usual, so here's the text inline, just in case the patch is broken:

    <attribute name="validatorClassName" required="false">
      <p>(String) The name of a class which implements the <code>org.apache.tomcat.jdbc.pool.Validator</code> interface and provides a no-arg constructor (may be implicit).
         If specified, the class will be used to create a Validator instance which is then used instead of any validation query to validate connections.
         The default value is <code>null</code>.
         An example value is <code>com.mycompany.project.SimpleValidator</code>.
      </p>
    </attribute>

Cheers. Patch applied. It will be in 1.0.9.0 onwards.

Thanks again.

May I ask why no provision was made for _configuring_ the Validator?  I love the pluggable validation approach, but even the JDBC4 isValid() scenario has a configurable element (the method's timeout value).  I agree that adding setValidatorClassName() makes sense for simple Resource configuration where appropriate, but these being by nature "custom" is seems inevitable that a Validator would be likely need to have config injected.

Is there a reason that setValidator() had to be removed?  It would be the most straightforward path for those using IOC-container-wired pools, though it wouldn't help when using a Tomcat Resource definition.

The only mechanism I can see as things are now would to use a JdbcInterceptor to inject a configured Validator (since props can be set for JdbcInterceptors), but that seems a little convoluted.  And what would be the appropriate hook there?  One could use the poolStarted() event, but wouldn't the initial connections have been started and tested by then?  Or maybe at the first connection creation.

Could we not just make the Validator be wired in the same way as the JdbcInterceptor, where optional properties can be defined?  I can't envision a need for a _chain_ like the interceptors have, but configurable properties would be very useful.

I agree, I will add the setValidator method

I added this in r1097895

Gah!  To my previous comment (#15), I wired up a Validator class which also extends JdbcInterceptor (I'll attach separately).  Since ConnectionPool.init() instantiates each defined JdbcInterceptor and then calls the poolStarted() method, the idea was that the config values could be passed as properties of the JdbcInterceptor, which would then apply those properties to the Validator instance created by setValidatorClassName() in the poolStarted() hook.

Inexplicably, though, ConnectionPool.init() does NOT apply the configured JdbcInterceptor properties - it just "new"s up an instance and immediately calls poolStarted().  Why doesn't it apply the configuration properties like it does in ConnectionPool.setupConnection(PooledConnection)????  So I can't really think of an easy way to inject a configuration into a Validator without code changes to jdbc-pool.

I'll attempt to attach my example (but now non-functional) Validator implementation to the issue.

Created attachment 26944 [details]
Example JDBC4 Validator/JdbcInjector implementation

See my previous comment on this bug

To be clear, please note that the example in #19 will NOT function correctly due to the issues discussed in #18.  It was just to clarify the workaround I was attempting.

Thanks for adding setValidator() back!  Though that still won't help those using a  Tomcat Resource definition.  I don't feel qualified to proclaim a "best of" solution, particularly with Validator already in "release".  Probably the easiest solution would be to augment the internals of setValidatorClassName() to allow a config suffix like JdbcInterceptor.  Maybe an additional ConfigurableValidator interface to allow the params to be passed and leave the implementation to the implementer (as with JdbcInterceptor)?

I'm happy to work up a proposed patch, but I'd prefer someone else weigh in on the best of the many possible approaches.

Created attachment 26947 [details]
Patch ConnectionPool.java to apply JdbcInterceptor properties before calling JdbcInterceptor.poolStarted()

This proposed patch to ConnectionPool would at least enable the configure-Validator-via-JdbcInterceptor approach, and should probably be there anyway for other JdbcInterceptor scenarios unrelated to configuring Validator instances.

Patch added to r1098667

Created attachment 26948 [details]
Updated example implementation for Jdbc4 Validator

Updating earlier example JDBC4 Validator implementation.  I hadn't noticed that *applying* the configured properties is left to the implementation class.  I "assumed" some sort of bean-property/reflection thing.

Sorry, I was a total doofus for posting the earlier implementation without confirming that the property application actually happened.  I've now tested that this version works (depends on the ConnectionPool patch from my previous comment).

Finally, I'm satisfied that this is a "good-enough" solution for Validator configuration.  It's a little awkward/indirect, but it uses existing/tested/documented functionality, which is a Good Thing.  Thanks...

Now you're confusing me, why would a validator be an interceptor?

Created attachment 26955 [details]
Updated [again] example implementation for Jdbc4 Validator

Minor tweak to reflect on the implementation class itself instead of the java.sql.Connection interface.

(In reply to comment #25)
> Created attachment 26955 [details]
> Updated [again] example implementation for Jdbc4 Validator
> 
> Minor tweak to reflect on the implementation class itself instead of the
> java.sql.Connection interface.

As I mentioned before, why on would this be an interceptor?

Please stop posting new code to this bug. This bug has been closed and marked fixed as per the original bug.
Open a new bug for new bugs/features/enhancements