Commons Dbcp
  1. Commons Dbcp
  2. DBCP-3

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

    Details

    • Type: Bug Bug
    • Status: Closed
    • Priority: Major Major
    • Resolution: Fixed
    • Affects Version/s: Nightly Builds
    • Fix Version/s: 1.3
    • Labels:
      None
    • 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);

        Issue Links

          Activity

          Hide
          Dirk Verbeeck added a comment -

          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.

          Show
          Dirk Verbeeck added a comment - 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.
          Hide
          Adam Jenkins added a comment -

          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.

          Show
          Adam Jenkins added a comment - 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.
          Hide
          Mauro Botelho added a comment -

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

          Show
          Mauro Botelho added a comment - 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."
          Hide
          Austin Mills added a comment -
              • COM-1582 has been marked as a duplicate of this bug. ***
          Show
          Austin Mills added a comment - COM-1582 has been marked as a duplicate of this bug. ***
          Hide
          Jacob Zwiers added a comment -

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

          Show
          Jacob Zwiers added a comment - Note this is also related to 32441 - that one refers to the same problem with PoolablePreparedStatements.
          Hide
          Ilja Preuß added a comment -

          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.

          Show
          Ilja Preuß added a comment - 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.
          Hide
          Jerome Lacoste added a comment -

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

          Show
          Jerome Lacoste added a comment - Can this be fixed? It's an obvious violation of the API contract and breaks legacy applications. Thanks.
          Hide
          Sandy McArthur (from Bugzilla import) added a comment -

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

          Show
          Sandy McArthur (from Bugzilla import) added a comment - (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( )
          Hide
          Philippe Mouawad added a comment -

          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.

          Show
          Philippe Mouawad added a comment - 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.
          Hide
          James Ring added a comment -

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

          Show
          James Ring added a comment - Is there anything wrong with Adam Jenkin's patch that was posted nearly 3 years ago now?
          Hide
          Phil Steitz added a comment -

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

          Show
          Phil Steitz added a comment - This represents a behavior change that may break some existing code, so fix version is set to 1.3.
          Hide
          James Ring added a comment -

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

          Show
          James Ring added a comment - Shouldn't we at least apply the patch to the trunk so that it appears in our nightlies?
          Hide
          Henri Yandell added a comment -

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

          Show
          Henri Yandell added a comment - Maybe time to branch 1.2.2 off and have trunk be 1.3?
          Hide
          Phil Steitz added a comment -

          Fixed in r 557176.

          Show
          Phil Steitz added a comment - Fixed in r 557176.

            People

            • Assignee:
              Unassigned
              Reporter:
              Adam Jenkins
            • Votes:
              3 Vote for this issue
              Watchers:
              1 Start watching this issue

              Dates

              • Created:
                Updated:
                Resolved:

                Development