Commons Dbcp
  1. Commons Dbcp
  2. DBCP-28

[dbcp][PATCH] Connection leak in PoolableConnection.close() (Oracle 10g driver)

    Details

    • Type: Bug Bug
    • Status: Closed
    • Priority: Major Major
    • Resolution: Fixed
    • Affects Version/s: 1.2
    • Fix Version/s: 1.2.2
    • Labels:
      None
    • Environment:

      Operating System: All
      Platform: PC

      Description

      Mail from Hugh Winkler on commons-dev (15-2-2005)
      --------------------------------------------------
      PoolableConnection.close() does logic equivalent to :

      if ( isClosed())

      { throw new SQLException(.); }

      else

      { _pool.returnObject(this); }

      The isClosed() method is that of ancestor DelegatingConnection, and it does:
      if(_closed || _conn.isClosed()) { return true; }
      return false;


      Now nothing prevents the underlying connection from closing itself; that's
      why isClosed() checks _conn.isClosed() – "did you close yourself while I
      wasn't looking?" But in that case PoolableConnection never calls
      _pool.returnObject().

      I've got a query in Oracle 10g that causes Oracle's connection to close
      itself: the famous "end of file on connection" message causes the connection
      to enter the closed state. Doesn't take long to exhaust the pool.

      I think the logic we want in PoolableConnection.close() is like so:



      if ( _closed ){ // really ask, did *we* close the connection already throw new SQLException(.); } else { _pool.returnObject(this); }

      If I've got some logic wrong please stop me before I deploy that change
      here!

      1. TestUtils.java
        1 kB
        Philippe Mouawad
      2. ShowsLeaksIfCheckForIsClosed.java
        2 kB
        Philippe Mouawad
      3. ShowsBasicDataSourceBadNumActiveIfNoCheckForIsClosedAndDoubleClose.java
        2 kB
        Philippe Mouawad
      4. ASF.LICENSE.NOT.GRANTED--TestPoolableConnection.patch
        5 kB
        James Ring
      5. ASF.LICENSE.NOT.GRANTED--PoolableConnection.patch
        0.7 kB
        James Ring

        Activity

        Hide
        Dirk Verbeeck added a comment -

        You want to invalidate the pooled connection if the underlying connection is
        closed. Try the following PoolableConnection.close() method.

        public synchronized void close() throws SQLException {
        try {
        if (_conn.isClosed()) {
        // underlying connection closed behind our back
        try

        { _pool.invalidateObject(this); }

        catch (Exception ie)

        { // DO NOTHING }

        throw new SQLException("Cannot close connection (underlying
        connection already closed)");
        }
        } catch (SQLException e) {
        try

        { _pool.invalidateObject(this); }

        catch (Exception ie)

        { // DO NOTHING the original exception will be rethrown }

        throw new SQLNestedException("Cannot close connection (isClosed
        check failed)", e);
        }
        if (_closed)

        { throw new SQLException("Already closed."); }

        else {
        try

        { _pool.returnObject(this); }

        catch(SQLException e)

        { throw e; } catch(RuntimeException e) { throw e; }

        catch(Exception e)

        { throw new SQLNestedException("Cannot close connection (return to pool failed)", e); }

        }
        }

        Show
        Dirk Verbeeck added a comment - You want to invalidate the pooled connection if the underlying connection is closed. Try the following PoolableConnection.close() method. public synchronized void close() throws SQLException { try { if (_conn.isClosed()) { // underlying connection closed behind our back try { _pool.invalidateObject(this); } catch (Exception ie) { // DO NOTHING } throw new SQLException("Cannot close connection (underlying connection already closed)"); } } catch (SQLException e) { try { _pool.invalidateObject(this); } catch (Exception ie) { // DO NOTHING the original exception will be rethrown } throw new SQLNestedException("Cannot close connection (isClosed check failed)", e); } if (_closed) { throw new SQLException("Already closed."); } else { try { _pool.returnObject(this); } catch(SQLException e) { throw e; } catch(RuntimeException e) { throw e; } catch(Exception e) { throw new SQLNestedException("Cannot close connection (return to pool failed)", e); } } }
        Hide
        Huw Lewis added a comment -

        I also had some issues with this bug, has this bug solution been accepted yet?

        When will the new fix be included in a new release of dbcp?

        I recently fixed the problem myself with the code shown below for
        PoolableConection.close() (i didn't notice Dirk Verbeeck had already posted a
        solution),
        I just invalidate any connection found to be already closed.
        The JUnit tests all still pass with my fix, BUT if someone can see something
        wrong with it please let me know. I've been using it for a while now and it
        seems to have fixed the connection leak problem i was experiencing.

        public synchronized void close() throws SQLException {
        boolean isClosed = false;
        try

        { isClosed = isClosed(); }

        catch (SQLException e) {
        try

        { _pool.invalidateObject(this); } catch (Exception ie) { // DO NOTHING the original exception will be rethrown }
        throw new SQLNestedException("Cannot close connection (isClosed
        check failed)", e);
        }
        if (isClosed) {
        try { _pool.invalidateObject(this); }

        catch (Exception ie)

        { // DO NOTHING, "Already closed" exception thrown below }

        throw new SQLException("Already closed.");
        } else {
        try

        { _pool.returnObject(this); }

        catch(SQLException e)

        { throw e; } catch(RuntimeException e) { throw e; }

        catch(Exception e)

        { throw new SQLNestedException("Cannot close connection (return to pool failed)", e); }

        }
        }

        Show
        Huw Lewis added a comment - I also had some issues with this bug, has this bug solution been accepted yet? When will the new fix be included in a new release of dbcp? I recently fixed the problem myself with the code shown below for PoolableConection.close() (i didn't notice Dirk Verbeeck had already posted a solution), I just invalidate any connection found to be already closed. The JUnit tests all still pass with my fix, BUT if someone can see something wrong with it please let me know. I've been using it for a while now and it seems to have fixed the connection leak problem i was experiencing. public synchronized void close() throws SQLException { boolean isClosed = false; try { isClosed = isClosed(); } catch (SQLException e) { try { _pool.invalidateObject(this); } catch (Exception ie) { // DO NOTHING the original exception will be rethrown } throw new SQLNestedException("Cannot close connection (isClosed check failed)", e); } if (isClosed) { try { _pool.invalidateObject(this); } catch (Exception ie) { // DO NOTHING, "Already closed" exception thrown below } throw new SQLException("Already closed."); } else { try { _pool.returnObject(this); } catch(SQLException e) { throw e; } catch(RuntimeException e) { throw e; } catch(Exception e) { throw new SQLNestedException("Cannot close connection (return to pool failed)", e); } } }
        Hide
        Wouter Zelle added a comment -

        This problem also occurred for us and caused a great deal of trouble on our
        production systems. We fixed the problem by building a custom DBCP with Lewis'
        fix. However, it makes me wonder why a bug that was reported and (cleanly) fixed
        a year ago didn't make it into the mainline yet.

        Show
        Wouter Zelle added a comment - This problem also occurred for us and caused a great deal of trouble on our production systems. We fixed the problem by building a custom DBCP with Lewis' fix. However, it makes me wonder why a bug that was reported and (cleanly) fixed a year ago didn't make it into the mainline yet.
        Hide
        Rob Hasselbaum added a comment -

        We have also experienced this problem at my company. Is the DBCP project dead?

        Show
        Rob Hasselbaum added a comment - We have also experienced this problem at my company. Is the DBCP project dead?
        Hide
        James Ring added a comment -

        Created an attachment (id=17845)
        Huw Lewis' change in patch format

        Not tested beyond regression tests.

        Show
        James Ring added a comment - Created an attachment (id=17845) Huw Lewis' change in patch format Not tested beyond regression tests.
        Hide
        James Ring added a comment -

        There hasn't been a source code change to dbcp in like 12 months. If this patch fixes what seems
        like a pretty nasty bug, can we get dbcp pushed out to release? Maybe 1.2.2?

        Thanks!

        Regards,
        James

        Show
        James Ring added a comment - There hasn't been a source code change to dbcp in like 12 months. If this patch fixes what seems like a pretty nasty bug, can we get dbcp pushed out to release? Maybe 1.2.2? Thanks! Regards, James
        Hide
        James Ring added a comment -

        Created an attachment (id=17846)
        Possible testcase to illustrate bug

        Hi there,

        The testPoolableConnectionLeak method in this test case may illustrate the
        bug. It shows that the connection pool has a getNumActive() equal to 1 even
        after calling PoolableConnection.close() with already closed delegate
        connection.

        Applying Huw's patch shows no active objects in the connection pool after the
        connection is closed. This is what I'd expect to happen, but correct me if
        I'm wrong...

        I am new to DBCP and I might not completely understand the issue, so if
        somebody with more experience with these classes could check this out, that'd
        be cool.

        Thanks!

        Regards,
        James

        Show
        James Ring added a comment - Created an attachment (id=17846) Possible testcase to illustrate bug Hi there, The testPoolableConnectionLeak method in this test case may illustrate the bug. It shows that the connection pool has a getNumActive() equal to 1 even after calling PoolableConnection.close() with already closed delegate connection. Applying Huw's patch shows no active objects in the connection pool after the connection is closed. This is what I'd expect to happen, but correct me if I'm wrong... I am new to DBCP and I might not completely understand the issue, so if somebody with more experience with these classes could check this out, that'd be cool. Thanks! Regards, James
        Hide
        Sandy McArthur (from Bugzilla import) added a comment -

        James, the testPoolableConnectionLeak() in "Possible testcase to illustrate bug"
        has a problem I think. GenericObjectPool (and any non-custom ObjectPool) doesn't
        have any special knowledge about objects borrowed from it. That test cased:
        1. borrows an object from the pool, in this case a Connection instance
        2. closes said Connection
        3. asks the pool again how many objects were borrowed from it

        The ObjectPool.getNumActive() method simply returns how many objects were
        returned from ObjectPool.borrowObject() minus how many were returned with
        ObjectPool.returnObject(...) or were invalidated with
        ObjectPool.invalidateObject(...)

        For the ObjectPool to return the right value for getNumActive() you should
        invalidate the connection by adding a pool.invalidateObject(conn); just before
        the assertEquals().

        Now the added pool.invalidateObject(conn); line does thown an exception and it
        really shouldn't. The point of the ObjectPool.invalidateObject(...) method is
        that the object being invalidated is in a broken state and the
        PoolableObjectFactory.destroyObject(...) method should be prepared for anything.

        Show
        Sandy McArthur (from Bugzilla import) added a comment - James, the testPoolableConnectionLeak() in "Possible testcase to illustrate bug" has a problem I think. GenericObjectPool (and any non-custom ObjectPool) doesn't have any special knowledge about objects borrowed from it. That test cased: 1. borrows an object from the pool, in this case a Connection instance 2. closes said Connection 3. asks the pool again how many objects were borrowed from it The ObjectPool.getNumActive() method simply returns how many objects were returned from ObjectPool.borrowObject() minus how many were returned with ObjectPool.returnObject(...) or were invalidated with ObjectPool.invalidateObject(...) For the ObjectPool to return the right value for getNumActive() you should invalidate the connection by adding a pool.invalidateObject(conn); just before the assertEquals(). Now the added pool.invalidateObject(conn); line does thown an exception and it really shouldn't. The point of the ObjectPool.invalidateObject(...) method is that the object being invalidated is in a broken state and the PoolableObjectFactory.destroyObject(...) method should be prepared for anything.
        Hide
        Sandy McArthur (from Bugzilla import) added a comment -

        (In reply to comment #8)
        Nevermind, pretend I never said that.

        Show
        Sandy McArthur (from Bugzilla import) added a comment - (In reply to comment #8) Nevermind, pretend I never said that.
        Hide
        Phil Steitz added a comment -

        Patches (17845, 17846) applied. Thanks! Leaving ticket open pending additional
        user / developer validation. These changes will be reflected in nightly builds
        beginning 09-March-06.

        Show
        Phil Steitz added a comment - Patches (17845, 17846) applied. Thanks! Leaving ticket open pending additional user / developer validation. These changes will be reflected in nightly builds beginning 09-March-06.
        Hide
        Sandy McArthur (from Bugzilla import) added a comment -

        I added some comments as a reminder to what should be changed in a non-bugfix
        release. I think the correct changes would risk breaking compatability since the
        _pool is protected and the class could be subclassed.
        http://svn.apache.org/viewcvs.cgi/jakarta/commons/proper/dbcp/trunk/src/java/org/apache/commons/dbcp/PoolableConnection.java?rev=384536&r1=384516&r2=384536

        Other than that, I say close this bug.

        Show
        Sandy McArthur (from Bugzilla import) added a comment - I added some comments as a reminder to what should be changed in a non-bugfix release. I think the correct changes would risk breaking compatability since the _pool is protected and the class could be subclassed. http://svn.apache.org/viewcvs.cgi/jakarta/commons/proper/dbcp/trunk/src/java/org/apache/commons/dbcp/PoolableConnection.java?rev=384536&r1=384516&r2=384536 Other than that, I say close this bug.
        Hide
        Philippe Mouawad added a comment -

        Hello,
        The files contain an illustration of the problem that will occur when the the previous patches will be applied in 1.2.2:
        Case 1:
        The client calls isClosed() before closing a connection since commons-dbcp does not allow double close without throwing (see http://issues.apache.org/jira/browse/DBCP-3)
        => The problem is that since he calls isClosed, he encounters the same bug reported here because conn.isClosed() will return true and the client will not call close.
        if (!conn.isClosed())
        {
        try

        { conn.close(); }

        catch(Exception e){}
        }
        see what happens if the isClosed() in PoolableConnection returns true : ShowsLeaksIfCheckForIsClosed

        Case2:
        The client tries to find a solution, and calls conn.close() without checking isClosed(), but the problem is that the client is in a Persistence fwk and the client may have already called close, so he ends up calling close() twice, see what happens if the isClosed() in PoolableConnection returns true : ShowsBasicDataSourceBadNumActiveIfNoCheckForIsClosedAndDoubleClose

        So My question is, what can I do if the double close is not allowed by DBCP

        Show
        Philippe Mouawad added a comment - Hello, The files contain an illustration of the problem that will occur when the the previous patches will be applied in 1.2.2: Case 1: The client calls isClosed() before closing a connection since commons-dbcp does not allow double close without throwing (see http://issues.apache.org/jira/browse/DBCP-3 ) => The problem is that since he calls isClosed, he encounters the same bug reported here because conn.isClosed() will return true and the client will not call close. if (!conn.isClosed()) { try { conn.close(); } catch(Exception e){} } see what happens if the isClosed() in PoolableConnection returns true : ShowsLeaksIfCheckForIsClosed Case2: The client tries to find a solution, and calls conn.close() without checking isClosed(), but the problem is that the client is in a Persistence fwk and the client may have already called close, so he ends up calling close() twice, see what happens if the isClosed() in PoolableConnection returns true : ShowsBasicDataSourceBadNumActiveIfNoCheckForIsClosedAndDoubleClose So My question is, what can I do if the double close is not allowed by DBCP
        Hide
        Andrei Solntsev added a comment -

        We are having this problem in our Production system.
        Behind the NAT, any DB connection becomes expired after 30 minutes of inactivity.
        When calling method "close()" on it, it throws SQLException "already closed", but it's not returned to the pool.

        This problem still remains in DBCP 1.3
        I will create a new issue for DBCP 1.3

        Show
        Andrei Solntsev added a comment - We are having this problem in our Production system. Behind the NAT, any DB connection becomes expired after 30 minutes of inactivity. When calling method "close()" on it, it throws SQLException "already closed", but it's not returned to the pool. This problem still remains in DBCP 1.3 I will create a new issue for DBCP 1.3
        Hide
        Andrei Solntsev added a comment -

        Sorry about confusion with versions.
        This bug is present in DBCP 1.2.1, but it's fixed in DBCP 1.2.2 (which is not officially released yet).

        Show
        Andrei Solntsev added a comment - Sorry about confusion with versions. This bug is present in DBCP 1.2.1, but it's fixed in DBCP 1.2.2 (which is not officially released yet).

          People

          • Assignee:
            Unassigned
            Reporter:
            Dirk Verbeeck
          • Votes:
            5 Vote for this issue
            Watchers:
            0 Start watching this issue

            Dates

            • Created:
              Updated:
              Resolved:

              Development