Connection created by SingleConnectionDataSource with suppressClose=true always returns isClosed=false even if the target connection is closed

[barfoo4711]

If SingleConnectionDataSource is configured with suppressClose=true it will produce a Connection wrapped in CloseSuppressingInvocationHandler. CloseSuppressingInvocationHandler will always return false if the isClosed() is being triggered on the wrapped connection. This is ok until SingleConnectionDataSource#destroy() is triggered which closed the connection on the wrapped Connection.

I'd therefore suggest to change

/**
 * Invocation handler that suppresses close calls on JDBC Connections.
 */
private static class CloseSuppressingInvocationHandler implements InvocationHandler {

	private final Connection target;

	public CloseSuppressingInvocationHandler(Connection target) {
		this.target = target;
	}

	@Override
	@Nullable
	public Object invoke(Object proxy, Method method, Object[] args) throws Throwable {
		// Invocation on ConnectionProxy interface coming in...

		if (method.getName().equals("equals")) {
			// Only consider equal when proxies are identical.
			return (proxy == args[0]);
		}
		else if (method.getName().equals("hashCode")) {
			// Use hashCode of Connection proxy.
			return System.identityHashCode(proxy);
		}
		else if (method.getName().equals("unwrap")) {
			if (((Class<?>) args[0]).isInstance(proxy)) {
				return proxy;
			}
		}
		else if (method.getName().equals("isWrapperFor")) {
			if (((Class<?>) args[0]).isInstance(proxy)) {
				return true;
			}
		}
		else if (method.getName().equals("close")) {
			// Handle close method: don't pass the call on.
			return null;
		}
		else if (method.getName().equals("isClosed")) {
			return false;
		}
		else if (method.getName().equals("getTargetConnection")) {
			// Handle getTargetConnection method: return underlying Connection.
			return this.target;
		}

		// Invoke method on target Connection.
		try {
			return method.invoke(this.target, args);
		}
		catch (InvocationTargetException ex) {
			throw ex.getTargetException();
		}
	}
}

to

/**
 * Invocation handler that suppresses close calls on JDBC Connections.
 */
private static class CloseSuppressingInvocationHandler implements InvocationHandler {

	private final Connection target;

	public CloseSuppressingInvocationHandler(Connection target) {
		this.target = target;
	}

	@Override
	@Nullable
	public Object invoke(Object proxy, Method method, Object[] args) throws Throwable {
		// Invocation on ConnectionProxy interface coming in...

		if (method.getName().equals("equals")) {
			// Only consider equal when proxies are identical.
			return (proxy == args[0]);
		}
		else if (method.getName().equals("hashCode")) {
			// Use hashCode of Connection proxy.
			return System.identityHashCode(proxy);
		}
		else if (method.getName().equals("unwrap")) {
			if (((Class<?>) args[0]).isInstance(proxy)) {
				return proxy;
			}
		}
		else if (method.getName().equals("isWrapperFor")) {
			if (((Class<?>) args[0]).isInstance(proxy)) {
				return true;
			}
		}
		else if (method.getName().equals("close")) {
			// Handle close method: don't pass the call on.
			return null;
		}
		// else if (method.getName().equals("isClosed")) {
		//	return false;
		// }
		else if (method.getName().equals("getTargetConnection")) {
			// Handle getTargetConnection method: return underlying Connection.
			return this.target;
		}

		// Invoke method on target Connection.
		try {
			return method.invoke(this.target, args);
		}
		catch (InvocationTargetException ex) {
			throw ex.getTargetException();
		}
	}
}

Let me know if this change would be ok then I can create a pull request

[jhoeller]

This is a bit of semantic issue since JDBC defines isClosed as only guaranteed to return true after the (local) close method has been called with actual effect, which this particular close-suppressing handle explicitly avoids. External resource events not triggered against the local handle might not be reflected there. That's why clients shouldn't use isClosed for validity checks but rather just attempt an operation and react to exceptions instead, as suggested by the JDBC javadoc there.

Are you using isClosed for a specific purpose? As an alternative to changing our semantics there, we could also document our specific isClosed behavior in the javadoc.

[barfoo4711]

Actually it's some 3rd party code which does the following

if (!dbCon.isClosed())
{
   if (LOG.isDebugEnabled())
   {
	  LOG.debug("Closing connection: " + dbCon.getMetaData().getURL()); //$NON-NLS-1$
   }
   dbCon.close();
}

If debug logging is enabled this code throws an org.h2.jdbc.JdbcSQLNonTransientException: The object is already closed exception in our testcase.

I think that semantics should still be ok. I wouldn't change anything about the suppressClose logic - so ìsClosedwould still return false unlessSingleConnectionDataSource#destroy()` is triggered. Only after then it would return true.

[jhoeller]

Fair enough, let's adapt our semantics then, maybe even delegating isClosed directly in our invocation handler since it's such a companion to the (also explicitly handled) close call. I'll fix this ASAP, also considering it for a backport in our April round of releases.

[barfoo4711]

thank you for the very prompt fix!