Uploaded image for project: 'Commons Dbcp'
  1. Commons Dbcp
  2. DBCP-3

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

    Details

    • Type: Bug
    • Status: Closed
    • Priority: Major
    • Resolution: Fixed
    • Affects Version/s: Nightly Builds
    • Fix Version/s: 1.3
    • Labels:
      None
    • Environment:

      Operating System: All
      Platform: All

    • Bugzilla Id:
      23185

      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
          dirkv 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
          dirkv 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@thejenkins.org 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@thejenkins.org 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@e-botelho.com 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@e-botelho.com 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
          amills@sensorlogic.com Austin Mills added a comment -
              • COM-1582 has been marked as a duplicate of this bug. ***
          Show
          amills@sensorlogic.com Austin Mills added a comment - COM-1582 has been marked as a duplicate of this bug. ***
          Hide
          apache_bugzilla@zwiers.ca Jacob Zwiers added a comment -

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

          Show
          apache_bugzilla@zwiers.ca Jacob Zwiers added a comment - Note this is also related to 32441 - that one refers to the same problem with PoolablePreparedStatements.
          Hide
          preuss@disy.net 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
          preuss@disy.net 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
          lacostej Jerome Lacoste added a comment -

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

          Show
          lacostej Jerome Lacoste added a comment - Can this be fixed? It's an obvious violation of the API contract and breaks legacy applications. Thanks.
          Hide
          sandymac@apache.org 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
          sandymac@apache.org 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
          p.mouawad@ubik-ingenierie.com 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
          p.mouawad@ubik-ingenierie.com 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
          sjr James Ring added a comment -

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

          Show
          sjr James Ring added a comment - Is there anything wrong with Adam Jenkin's patch that was posted nearly 3 years ago now?
          Hide
          psteitz 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
          psteitz 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
          sjr James Ring added a comment -

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

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

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

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

          Fixed in r 557176.

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

            People

            • Assignee:
              Unassigned
              Reporter:
              adam@thejenkins.org Adam Jenkins
            • Votes:
              3 Vote for this issue
              Watchers:
              1 Start watching this issue

              Dates

              • Created:
                Updated:
                Resolved:

                Development