commons-dev mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From Gary Gregory <garydgreg...@gmail.com>
Subject Re: [DBCP] isClosed contract (DBCP-398)
Date Mon, 27 May 2013 18:10:41 GMT
On Mon, May 27, 2013 at 10:49 AM, Phil Steitz <phil.steitz@gmail.com> 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.
>

This is quite a pickle. The current (DelegatingConnection) behavior feels
contradictory to the Javadoc of the JDBC interface:
http://docs.oracle.com/javase/6/docs/api/java/sql/Connection.html#isClosed%28%29

"A connection is closed if the method close has been called on it or if
certain fatal errors have occurred. This method is guaranteed to
return trueonly when it is called after the method
Connection.close has been called.This method generally cannot be called to
determine whether a connection to a database is valid or invalid. A typical
client can determine that a connection is invalid by catching any
exceptions that might be thrown when an operation is attempted."

Since we implement Connection, we should be able to decide what to return
from isClosed without delegating to the underlying connection. The current
solution feels like we are trying to cover our bases and return a value as
closest to the truth, but IMO if we have all of our ducks in a row, we
should be able to determine the return value without delegation.

"DBCP clients get true returned even if they have not called close on the
handle."

I am surprised this has not been reported before, so it must be a rare
corner case for users. The case when isClosed returns an exception should
be rare; to me that means 'the system is so messed up, I cannot even tell
what state I'm in.'...

Which brings me back around to this Javadoc fragment: "or if certain fatal
errors have occurred" and wondering if we would /not/ be implementing this
part of the spec if we just returned the state of our 'closed' ivar. As the
implementor of Connection.isClosed, don't we get to decide what "fatal
errors have occurred"? I think so. So this gives us the leeway to detect
errors and set the closed ivar as we see fit. Calling the the underlying is
closed may not be the right thing to do, I am not sure...

To further muddle things up, Java 6 introduced isValid():
http://docs.oracle.com/javase/6/docs/api/java/sql/Connection.html#isValid%28int%29which
is indirectly referred to from the isClosed Javadoc "A typical client
can determine that a connection is invalid by catching any exceptions that
might be thrown when an operation is attempted"

This makes it clearer (a little) to me that isClosed is really more about
the state of the connection /object/ and less about the ability to /use/
the database connection in a meaningful way, what's what isValid() is for.



> 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 does not seem that useful to me because when I am calling close, I am
announcing that I am done and that the connection can release its
resources. If close throws an exception, what am supposed to do with that?
Retry later? Maybe, in some case like 'you can't close now because I am
busy doing work on behalf of another thread' or some such. But in this
case, the connection is really closed, so I do not see what recoverable
action I can take. What does this exception in the case where the
connection was closed on the server give me the opportunity to do? It does
not seem that it does let me do anything useful in my application, or am I
missing a scenario?

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.
>
> 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).
>

This kind of fix feels like a minor point release (1.4 -> 1.5). I'm not
sure what that means for 1.3 unless we do (1.3 -> 1.5 and 1.4 -> 1.6)

Gary


> "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.
>
> Phil
>
> ---------------------------------------------------------------------
> To unsubscribe, e-mail: dev-unsubscribe@commons.apache.org
> For additional commands, e-mail: dev-help@commons.apache.org
>
>


-- 
E-Mail: garydgregory@gmail.com | ggregory@apache.org
Java Persistence with Hibernate, Second Edition<http://www.manning.com/bauer3/>
JUnit in Action, Second Edition <http://www.manning.com/tahchiev/>
Spring Batch in Action <http://www.manning.com/templier/>
Blog: http://garygregory.wordpress.com
Home: http://garygregory.com/
Tweet! http://twitter.com/GaryGregory

Mime
  • Unnamed multipart/alternative (inline, None, 0 bytes)
View raw message