vibur / vibur-dbcp

Vibur DBCP - concurrent and dynamic JDBC connection pool
http://www.vibur.org
Apache License 2.0
96 stars 11 forks source link

Validate connection before usage #12

Closed MrKuip closed 5 years ago

MrKuip commented 5 years ago

We are using mysql connector for jdbc. It's implementation of java.sql.connection caches the timezone. In our application we offer the possibility to change the default TimeZone AFTER a connection has been obtained. So we now have a pool of connections with the wrong TimeZone. We need to close those connections and remove them from the pool.

I now have a GetConnection() hook that throws a ViburDBCPException. I get a new Connection (this is good!) but the Connection that throws the ViburDBCPException is not removed from the pool and is not closed. How can I achieve that?

simeonmalchev commented 5 years ago

Hi @MrKuip,

Generally speaking it is a responsibility of the application to restore the state of a connection after use to what it was before use. I mean, if the application changes the connection timezone during use then the application has to restore the connection timezone to what it was before use. Best place to restore state will be in a CloseConnection() hook.

I'm not sure what exactly ViburDBCPException you're seeing in the GetConnection() hook? Can you post the full stacktrace of the exception here?

MrKuip commented 5 years ago

Edit: I'm not changing the timezone of the connection. I'm changing the default timezone of the whole application. TimeZone.setDefault()!

The TimeZone cannot be restored. MySQL stores the timezone in the connection. The connection should be discarded and a new one should be created.

I thought I had a solution for this problem and I created a GetConnection() hook that throws a VIburDBCPException when the TimeZone in the connection is different than the current default. But this doesn't work.

I'm looking for a hook to validate of the connection before it is used. If the validation fails it should be removed from the pool.

simeonmalchev commented 5 years ago

If in particular circumstances you need to completely discard a connection from the pool what you can do is instead of calling connection.close() call viburDataSource.severConnection(connection), see the javadoc here: https://github.com/vibur/vibur-dbcp/blob/master/src/main/java/org/vibur/dbcp/ViburDataSource.java#L130 . In this case you don't need any hooks, just replace the call to connection.close() with the one above. However, keep in mind that if your application needs to do this very often, you will completely lose the benefits of connections-pooling. If your application needs to do so rarely only then it is probably okay.

MrKuip commented 5 years ago

Thank you for the replies. We only change the TimeZone once (We want the client to have the same Timezone as our applicationserver)

I read the sourcecode and thought the best place was to validate was in the ConnectionFactory.readyToTake(). So I subclassed ConnectionFactory and added my validation to the readyToTake(). It works now but subclassing doesn't feel right.

In my further investigation I saw that in version 17.0 this was a the place for the Hook.ValidateConnection. But this Hook was removed. That would have been perfect for my usecase.

I will see if I can get a cleaner result with severConnection()

simeonmalchev commented 5 years ago

I removed Hook.ValidateConnection back in the days as I felt it was over designed and I didn't have any real-life scenario that was using it. Generally speaking, connection validation shouldn't be done before each connection use as it typically involves a database round trip which could be expensive; it is designed to be used once in a while, if the connection has stayed unused in the pool for, say, 15 seconds or more.

If severConnection() doesn't work well for you, consider doing validation / throwing of an SQLException from Hook.CloseConnection. An exception thrown by this hook will prevent the connection from being restored / included back in the pool.

MrKuip commented 5 years ago

Throwing a SQLException in Hook.CloseConnection works and is a nice clean solution. Thanks!

simeonmalchev commented 5 years ago

Btw, if you finish using Vibur in production and if it does good job for you, consider starring the project in your github account.

MrKuip commented 5 years ago

I'm trying to move away from dbcp but every ConnectionPool I try has issues.

I'm now looking at the testConnectionQuery. In dbcp we use a "select 1" to keep the connection alive but it looks like Vibur uses it to validate before usage. At some point in time MySQL closes connections that have not been used for some time. In our application that happens during the weekend when the factory is closed. We do not want MySQL to close the connection and therefore issue a 'select 1' on some idletime . Does Vibur have a comparable option?

simeonmalchev commented 5 years ago

No, Vibur doesn't have such built-in feature. I think that you can try to set on jdbcUrl level option similar to keepAlive=true but this option probably does keep-alive on socket level so it is probably not what you need. You can try to implement such feature on app level. Just over weekend have a scheduled job that takes in a loop N connections where N is the number of connections in the pool, execute on each connection "select 1" or similar, and then close the connection, i.e., return it to the pool; this should do the job in my opinion.

MrKuip commented 5 years ago

My opinion is that this feature should be provided by the connectionprovider. I really like your code (It's nice and clean) but right now I'm leaning towards c3p0 (ugly and messy) because of it's features.

Is there a possibility to add the feature "Keepalive idle connections"? And while i'm asking reinstating Hook.ValidateConnection?

simeonmalchev commented 5 years ago

I'm probably not looking to add keepAlive functionality to Vibur. From my point of view, this is application specific logic for some specific application use-cases.

I may consider to add back the Hook.ValidateConnection functionality in future but at the moment I cannot says if and when this will be added.