On 27/05/2013 15:49, Phil Steitz wrote: > We need to make a decision on whether or not to change the contract > for PoolableConnection (or DelegatingConnection or PoolGuardWrapper) > isClosed. There are two changes suggested in DBCP-398: > > 1) Currently, isClosed returns true if the connection handle has > been closed by the client *or* the underlying connection's isClosed > returns true. So if a connection has been closed server-side and > the driver reports this via isClosed on the physical connection, > DBCP clients get true returned even if they have not called close on > the handle. This behavior goes back to DBCP 1.0.
My reading of the java.sql.Connection Javadoc is that this is the correct behaviour. The Javadoc states: <quote> A connection is closed if the method close has been called on it or if certain fatal errors have occurred. </unquote> Looking at the implementation of this for DelegatingConnection: return _closed || _conn.isClosed(); The check of _closed is necessary to satisfy "if the close method has been called" and _conn.isClosed() is necessary to satisfy "or if certain fatal errors have occurred" The client should not be using isClosed() to determine whether or not to return the object to the pool because this is guaranteed to get a leak if "certain fatal errors have occurred". The client should return the object to the pool every time and let the pool handle any invalid connections. > 2) When a PoolableConnection is closed by the client and it has > already been closed server-side (i.e. the driver's isClosed returns > true on the physical connection) an SQLException is thrown. This > behavior is newly distinguished in DBCP 1.3/1.4. Prior to the > latest releases, if isClosed (with semantics above) returned true, > SQLException was always thrown. In 1.3/1.4, we made close > idempotent, so multiple client-side calls to close no longer > generate an SQLException. We decided, however, to still throw when > close is invoked on a connection handle whose underlying physical > connection's isClosed returns true. That seems reasonable to me. isClosed() is permitted to throw an SQLException so the clients have to be able to handle this. The root cause of the unexpected closure may or may not be an issue for the client. It could be expected (e.g. database was restarted) or it could be unexpected (indicative of a network issue between client and database). It is therefore important that the exception is passed to the client. Generally, there is little the client can do with an exception in this method apart from log it but that is probably what the client should be doing unless they really don't care about these errors. > As of now, 1.X and 2.0 code behave the same way. We have several > alternatives to choose from here: > > a) Do nothing - i.e., keep the contract as it is > b) "Fix" in 2.0 > c) Fix in 1.X > > If we do change the contract in 1.X, I am not sure it is OK to do > that in 1.3.1/1.4.1 - i.e. would have to move to 1.5, 1.7 or > something (recall that 1.3 is for JDK 1.4-1.5, 1.4 is JDK 1.6). > > "Fixing" 2) is straightforward. For 1), my inclination would be to > just change DelegatingConnection#isClosed; but logically the least > change approach would be changing PoolGuardConnectionWrapper > (followed by PoolableConnection, DelegatingConnection). > > Whatever we decide, we should make sure to document it. > Unfortunately, current behavior is not spelled out in the javadoc. > > It would be great to get some others' feedback on how best to proceed. My view is that the current code conforms to the Javadoc for java.sql.Connection and should not be changed. I'd have no objection to improving our own Javadoc to spell out that isClosed() should not be used as a test to determine if a connection should be closed (i.e. returned to the pool) or not. Mark --------------------------------------------------------------------- To unsubscribe, e-mail: [email protected] For additional commands, e-mail: [email protected]
