Commons Dbcp
  1. Commons Dbcp
  2. DBCP-216

Improvement of error recovery in KeyedCPDSConnectionFactory

    Details

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

      Windows XP, Java 1.5.0_06-b05, Sybase ASE 12.5.4, jConnect 6.0.5 EBF 13862, Commons Pool 1.3

      Description

      Attached you'll find a patch that improves the recovery of the class in different error situations.

      1. The addition of removeConnectionEventListener() in destroyObject() ensures that the listener is removed, which is not guaranteed to have happened upon call of destroyObject(). We are for sure not interested any more in events, since we are about to destroy.

      2. The same addition is made to connectionClosed(). Additionally, we have substituted there the call to destroyObject() with a call to pool.invalidateObject(). This is necessary because otherwise the object is destroyed but not removed from the pool.

      3. The same substitution is made in connectionErrorOccurred(), otherwise the object might remain in the pool.

        Activity

        Hide
        Phil Steitz added a comment -

        Completed fix in r814240

        1) Modified fix applied in r598045:

        • Removed workaround to prevent ConcurrentModificationExceptions generated by PooledConnectionImpl's notifyListeners method.
        • Changed PooledConnectionImpl notifyListeners to copy listeners and iterate over the copy instead of directly iterating over failfast Vector iterator.

        2) Applied the same fix to CPDSConnectionFactory

        Show
        Phil Steitz added a comment - Completed fix in r814240 1) Modified fix applied in r598045: Removed workaround to prevent ConcurrentModificationExceptions generated by PooledConnectionImpl's notifyListeners method. Changed PooledConnectionImpl notifyListeners to copy listeners and iterate over the copy instead of directly iterating over failfast Vector iterator. 2) Applied the same fix to CPDSConnectionFactory
        Hide
        Phil Steitz added a comment -

        Modified patch applied in r598045.

        To eliminate the potential for ConcurrentModificationExcepotions (which PooledConnectionImpl was throwing), I removed the listener removal from connection event handlers. This required refactoring to decouple listener and pcMap cleanup from connection event handling. Cleanup is now performed in makeObject.

        Leaving issue open, since similar changes should be made for CPDSConnectionFactory.

        Show
        Phil Steitz added a comment - Modified patch applied in r598045. To eliminate the potential for ConcurrentModificationExcepotions (which PooledConnectionImpl was throwing), I removed the listener removal from connection event handlers. This required refactoring to decouple listener and pcMap cleanup from connection event handling. Cleanup is now performed in makeObject. Leaving issue open, since similar changes should be made for CPDSConnectionFactory.
        Hide
        Phil Steitz added a comment -

        From commons-dev post by Stephane Demurget 13-nov-07. Shows not removing listener when connection is removed from pc map causes an exception when a connection is returned when maxIdle is reached.

        Thread [pool-5-thread-4] (Suspended (exception IllegalStateException))
        KeyedCPDSConnectionFactory.connectionClosed(ConnectionEvent) line:
        265
        SybPooledConnection.notifyListeners(SQLException) line: 225
        SybConnectionProxy.close() line: 106
        SybPooledConnection.close() line: 166
        KeyedCPDSConnectionFactory.destroyObject(Object, Object) line: 175

        GenericKeyedObjectPool.returnObject(Object, Object) line: 997
        KeyedCPDSConnectionFactory.connectionClosed(ConnectionEvent) line:
        268
        SybPooledConnection.notifyListeners(SQLException) line: 225
        SybConnectionProxy.close() line: 106
        ... (i'm calling close on the SQL Connection here, when the thread
        is finished)

        After analyzing it a bit I understand the pool wants to destroy the object
        because my pool can not support any more idle connection (I'm at the point
        where active=maxActive=maxIdle=5).
        What I do not understand is why the destroyObject method does:

        public void destroyObject(Object key, Object obj) throws Exception {
        if (obj instanceof PooledConnectionAndInfo)

        { PooledConnection pc = ((PooledConnectionAndInfo)obj).getPooledConnection(); pcMap.remove(pc); pc.close(); }

        }

        Which means it removes the mapping and close the PooledConnection. At this
        point, the listener is still active and it calls the listener which will
        obviously fail:

        public void connectionClosed(ConnectionEvent event) {
        PooledConnection pc = (PooledConnection)event.getSource();
        // if this event occured becase we were validating, ignore it
        // otherwise return the connection to the pool.
        if (!validatingMap.containsKey(pc)) {
        PooledConnectionAndInfo info =
        (PooledConnectionAndInfo) pcMap.get(pc);
        if (info == null)

        { throw new IllegalStateException(NO_KEY_MESSAGE); // <--------- the Exception which is thrown }
        Show
        Phil Steitz added a comment - From commons-dev post by Stephane Demurget 13-nov-07. Shows not removing listener when connection is removed from pc map causes an exception when a connection is returned when maxIdle is reached. Thread [pool-5-thread-4] (Suspended (exception IllegalStateException)) KeyedCPDSConnectionFactory.connectionClosed(ConnectionEvent) line: 265 SybPooledConnection.notifyListeners(SQLException) line: 225 SybConnectionProxy.close() line: 106 SybPooledConnection.close() line: 166 KeyedCPDSConnectionFactory.destroyObject(Object, Object) line: 175 GenericKeyedObjectPool.returnObject(Object, Object) line: 997 KeyedCPDSConnectionFactory.connectionClosed(ConnectionEvent) line: 268 SybPooledConnection.notifyListeners(SQLException) line: 225 SybConnectionProxy.close() line: 106 ... (i'm calling close on the SQL Connection here, when the thread is finished) After analyzing it a bit I understand the pool wants to destroy the object because my pool can not support any more idle connection (I'm at the point where active=maxActive=maxIdle=5). What I do not understand is why the destroyObject method does: public void destroyObject(Object key, Object obj) throws Exception { if (obj instanceof PooledConnectionAndInfo) { PooledConnection pc = ((PooledConnectionAndInfo)obj).getPooledConnection(); pcMap.remove(pc); pc.close(); } } Which means it removes the mapping and close the PooledConnection. At this point, the listener is still active and it calls the listener which will obviously fail: public void connectionClosed(ConnectionEvent event) { PooledConnection pc = (PooledConnection)event.getSource(); // if this event occured becase we were validating, ignore it // otherwise return the connection to the pool. if (!validatingMap.containsKey(pc)) { PooledConnectionAndInfo info = (PooledConnectionAndInfo) pcMap.get(pc); if (info == null) { throw new IllegalStateException(NO_KEY_MESSAGE); // <--------- the Exception which is thrown }
        Hide
        Phil Steitz added a comment -

        Patch looks correct and appropriate to me, subject to the comments below. Similar changes should probably be made to CPDSConnectionFactory.

        Regarding 2., with current backing pool (o.a.c.pool.GenericKeyedObjectPool), the destroy -> invalidate change (2.) will only work for active (i.e., checked out) connections and in this case it is necessary, but not to "remove from the pool," but to maintain integrity of the active count. Unfortunately, the contract of the pool's invalidate method only applies to objects that have been borrowed from the pool, so if the (exception-generating) connectionClosed event originates from from an idle connection, this will not work. This should not happen in general though, since these events should come from handles from checked out connections.

        Test cases illustrating the problem in this ticket should be added before committing the patch.

        Show
        Phil Steitz added a comment - Patch looks correct and appropriate to me, subject to the comments below. Similar changes should probably be made to CPDSConnectionFactory. Regarding 2., with current backing pool (o.a.c.pool.GenericKeyedObjectPool), the destroy -> invalidate change (2.) will only work for active (i.e., checked out) connections and in this case it is necessary, but not to "remove from the pool," but to maintain integrity of the active count. Unfortunately, the contract of the pool's invalidate method only applies to objects that have been borrowed from the pool, so if the (exception-generating) connectionClosed event originates from from an idle connection, this will not work. This should not happen in general though, since these events should come from handles from checked out connections. Test cases illustrating the problem in this ticket should be added before committing the patch.

          People

          • Assignee:
            Phil Steitz
            Reporter:
            Marcos Sanz
          • Votes:
            0 Vote for this issue
            Watchers:
            0 Start watching this issue

            Dates

            • Created:
              Updated:
              Resolved:

              Development