DataSourceUtils does not respect c3p0 TimeoutException [SPR-7226]

[spring-projects-issues]

Janning Vygen opened SPR-7226 and commented

See DataSourceUtils.prepareConnectionForTransaction(Connection con, TransactionDefinition definition)

In the catch Block every Exception is catched and silently dropped:

// Set read-only flag.
if (definition != null && definition.isReadOnly()) {
try {
if (logger.isDebugEnabled()) {
logger.debug("Setting JDBC Connection [" + con + "] read-only");
}
con.setReadOnly(true);
}
catch (Throwable ex) {
// SQLException or UnsupportedOperationException
// -> ignore, it's just a hint anyway.
logger.debug("Could not set JDBC Connection read-only", ex);
}
}

In the case where i have c3p0.checkoutTimeout set, it silently drops the TimeoutException here.

The TimeoutConnection get thrown when Hibernate trys to start a connection (in doBeginTransaction()). So when your database is overloaded and you have a timeout of 3 Seconds it takes an effective time of 6 seconds to get the exception, when your transaction isReadOnly(). This is annoying, when you really want to have a timeout of 3 seconds in all cases.

---

Affects: 3.0.2

Referenced from: commits 0c6b38b, 8800bab, d7f72fb

[spring-projects-issues]

Juergen Hoeller commented

I wasn't aware that any timeout exceptions get thrown from setReadOnly calls... Timeouts usually get checked when the first query is being executed on a Connection. Anyway, what exactly is the exception thrown from setReadOnly in the C3P0 case? C3P0's TimeoutException seems to be a regular checked exception, so I suppose it comes back from setReadOnly as a wrapped SQLException or RuntimeException?

Juergen

[spring-projects-issues]

Janning Vygen commented

You are right. com.mchange.v2.resourcepool.TimeoutException gets wrapped into an SQLException
The timeoutException get thrown in com.mchange.v2.resourcepool.BasicResourcePool awaitAvailable(long timeout), but it gets wrapped inside
com.mchange.v2.c3p0.impl.C3P0PooledConnectionPool:

public PooledConnection checkoutPooledConnection() throws SQLException
{
//System.err.println(this + " -- CHECKOUT");
try { return (PooledConnection) rp.checkoutResource( checkoutTimeout ); }
catch (TimeoutException e)
{ throw SqlUtils.toSQLException("An attempt by a client to checkout a Connection has timed out.", e); }

[spring-projects-issues]

Juergen Hoeller commented

Alright, that helps. We're checking for an exception class with "Timeout" in the name now (e.g. JDBC 4.0's SQLTimeoutException), and also for an exception's cause with "Timeout" in the name (like in C3P0's case, where the primary exception is a plain SQLException). Such a timeout exception will get propagated, whereas all other setReadOnly exceptions will get swallowed and logged at debug level.

Juergen

[spring-projects-issues]

Juergen Hoeller commented

This will be available in tomorrow's 3.0.3 snapshot. Feel free to give it an early try.

Juergen

[spring-projects-issues]

Janning Vygen commented

Your patch is not working, because my information was incomplete.
Let me explain the situation further:

First i am talking about a TimeoutException which is caused by not getting a Database connection, when the resourc epool is exhausted.
I am not talking about TimeoutConnection because transaction takes too long. Just to clearify if this was a misunderstanding about this situation.

So what i first get is a SQLException with a wrapped TimeoutException
com.mchange.v2.resourcepool.TimeoutException: A client timed out while waiting to acquire a resource from com.mchange.v2.resourcepool.BasicResourcePool@3e43ca1 -- timeout at awaitAvailable()

But this Exception gets wrapped into an GenericJDBCException by openConnection() in org.hibernate.jdbc.ConnectionManager line 449

if (ex instanceof SQLException && (ex.getClass().getSimpleName().contains("Timeout") ||
              (ex.getCause() != null && ex.getCause().getClass().getSimpleName().contains("Timeout")))) {
     // Assume it's a connection timeout that would otherwise get lost: e.g. from C3P0.
     throw (SQLException) ex;
}

[spring-projects-issues]

Janning Vygen commented

Sorry my last comment was mistakenly submitted. I didn't want to post the last block of code. i couldn't find a way to edit my comment.

For c3p0 the catch block should look like this:

catch (Throwable ex) {
if (ex instanceof GenericJDBCException) {
SQLException sqle = ex.getSQLException();
if (sqle.getCause != null && sqle.getCause().getClass().getSimpleName().contains("Timeout")) {
throw ex;
}
}
// "read-only not supported" SQLException or UnsupportedOperationException
// -> ignore, it's just a hint anyway.
logger.debug("Could not set JDBC Connection read-only", ex);
}

[spring-projects-issues]

Juergen Hoeller commented

I've committed a revised version of this, searching any level of cause. Will be available in tonight's 3.0.3 snapshot!

Juergen

[spring-projects-issues]

Janning Vygen commented

great! works for me!