Commons Dbcp
  1. Commons Dbcp
  2. DBCP-358

Equals implementations in DelegatingXxx classes are not symmetric

    Details

    • Type: Bug Bug
    • Status: Closed
    • Priority: Major Major
    • Resolution: Fixed
    • Affects Version/s: 1.2, 1.2.2, 1.3, 1.4
    • Fix Version/s: 2.0
    • Labels:
      None

      Description

      For reasons unclear to me, DelegatingConnection, DelegatingStatement, PoolGuardConnectionWrappers and other DBCP classes implement equals so that the wrapping class is considered equal to its innermost delegate JDBC object. This makes equals asymmetric when applied to a wrapper and its wrapped JDBC object - wrapper.equals(delegate) returns true, but delegate.equals(wrapper) will in general return false.

      I am pretty sure that DBCP itself does not rely on this bugged behavior, so I am inclined to fix it, making equals an equivalence relation on wrapper instances, with two considered equal iff their innermost delegates are equal. I can't imagine use cases where the bugged behavior is required. Can anyone else?

        Issue Links

          Activity

          Hide
          Joerg Schaible added a comment -

          This behavior is unfortunately used in the PoolableManagedConnection w/ TransactionRegistry. When a new connection is created in the pool, the unwrapped connection is registered (and used as key in the internal WeakHashMap). Later on the wrapped connection is used to unregister again (or to lookup the XAResource as done in org.apache.commons.dbcp.managed.TestBasicManagedDataSource.testReallyClose()). As workaround it is possible to wrap the registered connection if it is not wrapped yet:

          Index: src/java/org/apache/commons/dbcp/managed/TransactionRegistry.java
          ===================================================================
          --- src/java/org/apache/commons/dbcp/managed/TransactionRegistry.java   (revision 1098813)
          +++ src/java/org/apache/commons/dbcp/managed/TransactionRegistry.java   (working copy)
          @@ -28,7 +28,9 @@
           import javax.transaction.TransactionManager;
           import javax.transaction.xa.XAResource;
           
          +import org.apache.commons.dbcp.DelegatingConnection;
           
          +
           /**
            * TransactionRegistry tracks Connections and XAResources in a transacted environment for a single XAConnectionFactory.
            * </p>
          @@ -62,7 +64,10 @@
               public synchronized void registerConnection(Connection connection, XAResource xaResource) {
                   if (connection == null) throw new NullPointerException("connection is null");
                   if (xaResource == null) throw new NullPointerException("xaResource is null");
          -        xaResources.put(connection, xaResource);
          +        Connection key = connection instanceof DelegatingConnection 
          +            ? connection 
          +            : new DelegatingConnection(connection);
          +        xaResources.put(key, xaResource);
               }
           
               /**
          

          This approach will also solve DBCP-356 (another workaround is to use a HasMap instead of a WeakHashMap for the XAResources).

          Show
          Joerg Schaible added a comment - This behavior is unfortunately used in the PoolableManagedConnection w/ TransactionRegistry. When a new connection is created in the pool, the unwrapped connection is registered (and used as key in the internal WeakHashMap). Later on the wrapped connection is used to unregister again (or to lookup the XAResource as done in org.apache.commons.dbcp.managed.TestBasicManagedDataSource.testReallyClose()). As workaround it is possible to wrap the registered connection if it is not wrapped yet: Index: src/java/org/apache/commons/dbcp/managed/TransactionRegistry.java =================================================================== --- src/java/org/apache/commons/dbcp/managed/TransactionRegistry.java (revision 1098813) +++ src/java/org/apache/commons/dbcp/managed/TransactionRegistry.java (working copy) @@ -28,7 +28,9 @@ import javax.transaction.TransactionManager; import javax.transaction.xa.XAResource; +import org.apache.commons.dbcp.DelegatingConnection; + /** * TransactionRegistry tracks Connections and XAResources in a transacted environment for a single XAConnectionFactory. * </p> @@ -62,7 +64,10 @@ public synchronized void registerConnection(Connection connection, XAResource xaResource) { if (connection == null) throw new NullPointerException("connection is null"); if (xaResource == null) throw new NullPointerException("xaResource is null"); - xaResources.put(connection, xaResource); + Connection key = connection instanceof DelegatingConnection + ? connection + : new DelegatingConnection(connection); + xaResources.put(key, xaResource); } /** This approach will also solve DBCP-356 (another workaround is to use a HasMap instead of a WeakHashMap for the XAResources).
          Hide
          Joerg Schaible added a comment -

          Thinking about it ... if I create a temporary connection instance and use it as key in a WeakHashMap, it does not make too much sense either. OTOH, if I use an unwrapped connection as key that is also part of the WeakHashMap's value (the xaResource), the WeakHashMap is also useless. So, what do we try to solve with the WeakHashMap for the XAResources here?

          Show
          Joerg Schaible added a comment - Thinking about it ... if I create a temporary connection instance and use it as key in a WeakHashMap, it does not make too much sense either. OTOH, if I use an unwrapped connection as key that is also part of the WeakHashMap's value (the xaResource), the WeakHashMap is also useless. So, what do we try to solve with the WeakHashMap for the XAResources here?
          Hide
          Balazs Zsoldos added a comment - - edited

          Would be nice to fix this issue in a way that equals and hashcode does not use innermost.

          A short example why a programmer feels the current behavior is a bug::

          Connection conn1 = dataSource.getConnection();
          conn1.close();
          Connection conn2 = dataSource.getConnection();
          if (conn2.equals(conn1) && conn1.isClosed() != conn2.isClosed()) {
              // Oops they are equal but one of them is closed but the other is not
          }
          
          Show
          Balazs Zsoldos added a comment - - edited Would be nice to fix this issue in a way that equals and hashcode does not use innermost. A short example why a programmer feels the current behavior is a bug:: Connection conn1 = dataSource.getConnection(); conn1.close(); Connection conn2 = dataSource.getConnection(); if (conn2.equals(conn1) && conn1.isClosed() != conn2.isClosed()) { // Oops they are equal but one of them is closed but the other is not }
          Hide
          Mark Thomas added a comment -

          I have a great deal of sympathy with the point that Balazs makes above. I intend to explore removing the custom equals and hashcode methods entirely. I know that this is going to break a bunch of tests. What I need to look at is whether the tests are just testing for the current behaviour or whether - like TransactionRegistry - the implementation actually depends on the current behaviour. Assuming it is the former, I'll change the tests. If it is the latter, I'll have to see what options I'm left with.

          Regarding the TransactionRegistry, there is going to have to be a hack of some form somewhere. My preferred approach at the moment is to make getInnermostDelegateInternal() public but make it clear in the Javadoc that it is not part of the API. What I like about this is that it becomes very clear that the code that depends on the innermost delegate rather than having a dependency on an equals implementation several hops away. This should also provide a route to change any other code that depends on the current equals and hashcode behaviour.

          Show
          Mark Thomas added a comment - I have a great deal of sympathy with the point that Balazs makes above. I intend to explore removing the custom equals and hashcode methods entirely. I know that this is going to break a bunch of tests. What I need to look at is whether the tests are just testing for the current behaviour or whether - like TransactionRegistry - the implementation actually depends on the current behaviour. Assuming it is the former, I'll change the tests. If it is the latter, I'll have to see what options I'm left with. Regarding the TransactionRegistry, there is going to have to be a hack of some form somewhere. My preferred approach at the moment is to make getInnermostDelegateInternal() public but make it clear in the Javadoc that it is not part of the API. What I like about this is that it becomes very clear that the code that depends on the innermost delegate rather than having a dependency on an equals implementation several hops away. This should also provide a route to change any other code that depends on the current equals and hashcode behaviour.
          Hide
          Mark Thomas added a comment -

          This can't be fixed prior to 2.0 because it requires API changes. It has been fixed in 2.0 onwards.

          The equals() implementations of the DelegatingXxx classes are now symmetric.
          There are some important API changes underlying this fix:

          • two DelegatingXxx instances are no longer considered equal if they have the same innermost delegate;
          • a DelegatingXxx instance is not considered equal to its innermost delegate;
          • the getInnermostDelegateInternal() method has been made public (but remains part of the internal API) to allow classes extending this implementation to access the innermost delegate when required.
          Show
          Mark Thomas added a comment - This can't be fixed prior to 2.0 because it requires API changes. It has been fixed in 2.0 onwards. The equals() implementations of the DelegatingXxx classes are now symmetric. There are some important API changes underlying this fix: two DelegatingXxx instances are no longer considered equal if they have the same innermost delegate; a DelegatingXxx instance is not considered equal to its innermost delegate; the getInnermostDelegateInternal() method has been made public (but remains part of the internal API) to allow classes extending this implementation to access the innermost delegate when required.

            People

            • Assignee:
              Unassigned
              Reporter:
              Phil Steitz
            • Votes:
              1 Vote for this issue
              Watchers:
              2 Start watching this issue

              Dates

              • Created:
                Updated:
                Resolved:

                Development