[dbcp] PoolableConnection.close() won't allow multiple close

• Key: DBCP-3
• Type: Bug
• Status: Closed
• Resolution: Fixed
• Priority: Major
• Assignee: Unassigned
• Reporter: Adam Jenkins
• Affects Version/s: Nightly Builds
• Fix Version/s: 1.3

Environment

Operating System: All
Platform: All

Description

Sun's javadoc for java.sql.Connection.close() specifies that calling close on an
already closed Connection is a no-op. However, PoolableConnection.close() (v
1.10) throws a SQLException if close() is called on a closed Connection.
PoolableConnection.close() should just return if the Connection is already
closed. Here is a patch:

To demonstrate the bug, just obtain an open PoolableConnection and call close()
on it twice; the second call will produce a SQLException. According to Sun's
spec, the second close() should just be a no-op. The current behaviour is
preferable to the old behaviour where it returned the Connection to the pool
twice, but it's still not according to the spec.

Here's a patch:

• • • PoolableConnection.java.orig 2003-09-15 16:07:53.000000000 -0400
    • PoolableConnection.java 2003-09-15 16:08:11.000000000 -0400
      ***************
    • 108,114 ****
      */
      public synchronized void close() throws SQLException {
      if(isClosed()) { ! throw new SQLException("Already closed."); } else {
      try {
      _pool.returnObject(this);
    • 108,114 ----
      */
      public synchronized void close() throws SQLException {
      if(isClosed()) { ! return; } else {
      try {
      _pool.returnObject(this);

Comments

[Dirk Verbeeck]

I believe this behaviour is intended.
There is a junit test that fails if no exception is thrown:
testCantCloseConnectionTwice()

Also the alternative datasource implementation does the same in
org.apache.commons.dbcp.cpdsadapter.ConnectionImpl

I'm marking this issue WONTFIX until more evidence is presented that throwing an
exception is illegal / undesired.

[Adam Jenkins]

For evidence that Connection.close() shouldn't throw an exception just because the connection is already closed, see Sun's API spec for the Connction.close() method. http://java.sun.com/j2se/1.4.2/docs/api/java/sql/Connection.html#close() The relevent quote is: "Calling the method close on a Connection object that is already closed is a no-op." That makes it pretty clear to me that the PoolableConnection.close implementation is not what the spec intended in this regard. Note, I can well believe that the current behaviour was intended, and I think good arguments can be made for the spec's behaviour and for PoolableConnection's behaviour. However, I don't think there's any great advantage to either implementation, so I think the spec should win out. I found this problem because we actually had code which called close twice; once when the connection was done being used, and later in the finally block in case there was an error before the normal close. We were using DriverManager but recently decided to use connection pooling, and so switched to commons-dbcp and started getting exceptions. If I'd written the code, I wouldn't have called Connection.close twice anyway, but I can't really fault the original programmer since he was following Sun's spec, so I must fault the PoolableConnection implementation.

[Mauro Botelho]

This is a bug. According to
http://java.sun.com/j2se/1.4.2/docs/api/java/sql/Statement.html#close()

"Calling the method close on a Statement object that is already closed has no
effect."

[Austin Mills]

• • • COM-1582 has been marked as a duplicate of this bug. ***

[JZ]

Note this is also related to 32441 - that one refers to the same problem with
PoolablePreparedStatements.

[Ilja Preuß]

Anyone working on this? This has been reported two years ago now, and it's
really a PITA when trying to introduce DBCP for a legacy app.

[Jerome Lacoste]

Can this be fixed? It's an obvious violation of the API contract and breaks
legacy applications. Thanks.

[Sandy McArthur (from Bugzilla import)]

(In reply to comment #7)
> Can this be fixed? It's an obvious violation of the API contract and breaks
> legacy applications. Thanks.

Yes, it needs to be fixed. Working on Dbcp is next on my personal todo list,
maybe someone else will get to it sooner.

In the existing code's defense the contract for close changed from Java 1.3 to 1.4:
http://java.sun.com/j2se/1.3/docs/api/java/sql/Connection.html#close()
http://java.sun.com/j2se/1.4.2/docs/api/java/sql/Connection.html#close()

[Philippe Mouawad]

See Comment of [Philippe Mouawad] 12/Jul/06 09:19 PM for a problem that occurs if this bug is not corrected.
http://issues.apache.org/jira/browse/DBCP-28

Philippe.

[James Ring]

Is there anything wrong with Adam Jenkin's patch that was posted nearly 3 years ago now?

[Phil Steitz]

This represents a behavior change that may break some existing code, so fix version is set to 1.3.

[James Ring]

Shouldn't we at least apply the patch to the trunk so that it appears in our nightlies?

[Henri Yandell]

Maybe time to branch 1.2.2 off and have trunk be 1.3?

[Phil Steitz]

Fixed in r 557176.