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

Connection proxy doesn't handle exceptions properly #8

Closed txshtkckr closed 7 years ago

txshtkckr commented 7 years ago
java.lang.reflect.UndeclaredThrowableException
    ... unrelated frames ...
Caused by: java.lang.reflect.InvocationTargetException
    at sun.reflect.DelegatingMethodAccessorImpl.invoke(DelegatingMethodAccessorImpl.java:43)
    at java.lang.reflect.Method.invoke(Method.java:498)
    ... unrelated frames ...
    at org.postgresql.jdbc2.AbstractJdbc2Connection.checkClosed(AbstractJdbc2Connection.java:822)
    at org.postgresql.jdbc2.AbstractJdbc2Connection.getAutoCommit(AbstractJdbc2Connection.java:788)
    at sun.reflect.DelegatingMethodAccessorImpl.invoke(DelegatingMethodAccessorImpl.java:43)
    at java.lang.reflect.Method.invoke(Method.java:498)
    at org.vibur.dbcp.proxy.AbstractInvocationHandler.targetInvoke(AbstractInvocationHandler.java:144)

AbstractInvocationHandle.targetInvoke should be catching InvocationTargetException and throwing its unwrapped getCause() instead of allowing it to escape the proxy, because it is a checked exception that the proxied methods do not declare, causing it to be re-wrapped as an UndeclaredThrowableException. This means that code that tries to catch SQLException fails to, meaning the pool is inadvertently altering the exception behaviour of the methods. It should not do that.

More detail is available at the internal Atlassian issue https://jdog.jira-dev.com/browse/JEX-30043

simeonmalchev commented 7 years ago

Hi Chris,

Unless if I have misunderstood something, targetInvoke() actually catches IvocationTargetException and then re-throws its clause. It will throw ViburDBCPException (that is a RuntimeException) only if the underlying cause is null which will be very strange. Is this what is happening?

final Object targetInvoke(Method method, Object[] args) throws SQLException { try { return method.invoke(target, args); // the real method call on the real underlying (proxied) object

} catch (InvocationTargetException e) {
    Throwable cause = e.getCause();
    if (cause == null)
        cause = e;

    logTargetInvokeFailure(method, args, cause);

    if (cause instanceof SQLException) {
        SQLException sqlException = (SQLException) cause;
        exceptionCollector.addException(sqlException);
        throw sqlException;
    }
    else if (cause instanceof RuntimeException)
        throw (RuntimeException) cause;
    else if (cause instanceof Error)
        throw (Error) cause;

    throw unexpectedException(e);
} catch (IllegalAccessException e) {
    throw unexpectedException(e);
}

}

private static ViburDBCPException unexpectedException(ReflectiveOperationException e) { logger.error("Unexpected exception cause", e); return new ViburDBCPException(e); // not expected to happen }

On Mon, Jul 24, 2017 at 2:18 AM, Chris Fuller notifications@github.com wrote:

java.lang.reflect.UndeclaredThrowableException ... unrelated frames ... Caused by: java.lang.reflect.InvocationTargetException at sun.reflect.DelegatingMethodAccessorImpl.invoke(DelegatingMethodAccessorImpl.java:43) at java.lang.reflect.Method.invoke(Method.java:498) ... unrelated frames ... at org.postgresql.jdbc2.AbstractJdbc2Connection.checkClosed(AbstractJdbc2Connection.java:822) at org.postgresql.jdbc2.AbstractJdbc2Connection.getAutoCommit(AbstractJdbc2Connection.java:788) at sun.reflect.DelegatingMethodAccessorImpl.invoke(DelegatingMethodAccessorImpl.java:43) at java.lang.reflect.Method.invoke(Method.java:498) at org.vibur.dbcp.proxy.AbstractInvocationHandler.targetInvoke(AbstractInvocationHandler.java:144)

AbstractInvocationHandle.targetInvoke should be catching InvocationTargetException and throwing its unwrapped getCause() instead of allowing it to escape the proxy, because it is a checked exception that the proxied methods do not declare, causing it to be re-wrapped as an UndeclaredThrowableException. This means that code that tries to catch SQLException fails to, meaning the pool is inadvertently altering the exception behaviour of the methods. It should not do that.

More detail is available at the internal Atlassian issue https://jdog.jira-dev.com/browse/JEX-30043

— You are receiving this because you are subscribed to this thread. Reply to this email directly, view it on GitHub https://github.com/vibur/vibur-dbcp/issues/8, or mute the thread https://github.com/notifications/unsubscribe-auth/AKG33gkkdAvQc-jLC3hF9Q_szdhXF0P4ks5sQ9TJgaJpZM4OgmgM .

txshtkckr commented 7 years ago

It turns out it was yet another dynamic proxy jumping in the middle that I had overlooked. The problem isn't in vibur.