On 5/28/13 1:58 AM, Mark Thomas wrote: > 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"
I guess that is defensible. The second clause is there for the kind of situation that we face when the underlying physical connection has been closed without the pool or pool client's involvement. > > 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. I agree with this. > >> 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. I agree here too. For 2.0, we may want to provide tracking for these as we do for validation failures; but I guess in 1.x it may be best to just leave as is. > >> 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. I suggest then that we add comments above to the DBCP-398 and close as WONT_FIX. Phil > > Mark > > > --------------------------------------------------------------------- > To unsubscribe, e-mail: [email protected] > For additional commands, e-mail: [email protected] > > --------------------------------------------------------------------- To unsubscribe, e-mail: [email protected] For additional commands, e-mail: [email protected]
