ActiveMQ
  1. ActiveMQ
  2. AMQ-3457

temp destinations should only be deleted once all users of a pooled connection call close

    Details

    • Type: Bug Bug
    • Status: Resolved
    • Priority: Major Major
    • Resolution: Fixed
    • Affects Version/s: 5.x
    • Fix Version/s: 5.6.0
    • Component/s: None
    • Labels:
      None
    • Environment:

      5.6-SNAPSHOT

    • Patch Info:
      Patch Available

      Description

      AMQ-2349 added some code to clean up temp destinations once close() is called on a pooled connection. This caused pretty much all the JMS request-reply tests to fail in Camel with "javax.jms.InvalidDestinationException: Cannot publish to a deleted Destination"

      The problem is that with a pooled connection, multiple users can be using a Connection at the same time (a reference count is kept of how many there are) so if once calls close() the temp destinations of several others could be deleted while they are still using them. I think the correct behavior would be to only delete the temp destinations when all connection users call close() (i.e. when the reference count becomes zero).

      Attaching a proposed fix for this shortly for review.

      1. amqpool.diff
        3 kB
        Jonathan Anstey
      2. pooledConnCleanupOwnTemps.patch
        7 kB
        Arthur Naseef
      3. PooledSessionTempDestEventListener.java
        0.5 kB
        Arthur Naseef
      4. TestConnectionPoolLingeringTemps.java
        6 kB
        Arthur Naseef
      5. TestConnectionPoolTempCleanup.java
        8 kB
        Arthur Naseef

        Activity

        Hide
        Arthur Naseef added a comment -

        Created AMQ-3680 to follow up. It turns out I can't re-open this ticket.

        Thanks!

        Show
        Arthur Naseef added a comment - Created AMQ-3680 to follow up. It turns out I can't re-open this ticket. Thanks!
        Hide
        Timothy Bish added a comment -

        You are welcome to reopen or create a new issue with your additional work. When someone has time to review the issue they will be able to determine the correct course of action.

        Show
        Timothy Bish added a comment - You are welcome to reopen or create a new issue with your additional work. When someone has time to review the issue they will be able to determine the correct course of action.
        Hide
        Arthur Naseef added a comment -

        Is there more I can do to facilitate this? I know I updated several times; I apologize for not providing cleaner packaging.

        Should I reopen this ticket, or open a new one?

        Show
        Arthur Naseef added a comment - Is there more I can do to facilitate this? I know I updated several times; I apologize for not providing cleaner packaging. Should I reopen this ticket, or open a new one?
        Hide
        Arthur Naseef added a comment -

        New source file that's missing from the patch and is needed for the solution.

        Show
        Arthur Naseef added a comment - New source file that's missing from the patch and is needed for the solution.
        Hide
        Arthur Naseef added a comment -

        More thorough JUnit - supercedes the TestConnectionPoolLingeringTemps.

        Corrects the assertion comments and adds a test that closing one PooledConnection does not delete the temporary destinations of a second one.

        Show
        Arthur Naseef added a comment - More thorough JUnit - supercedes the TestConnectionPoolLingeringTemps. Corrects the assertion comments and adds a test that closing one PooledConnection does not delete the temporary destinations of a second one.
        Hide
        Arthur Naseef added a comment -

        Third try - better remove the temp dests before decrementing the count on the connection pool in case that causes the underlying ActiveMQConnection to close.

        Show
        Arthur Naseef added a comment - Third try - better remove the temp dests before decrementing the count on the connection pool in case that causes the underlying ActiveMQConnection to close.
        Hide
        Arthur Naseef added a comment -

        Corrected patch - the original missed the all-important change to the PooledConnection.close() method itself.

        This was tested with the JUnit case provided and is working.

        Show
        Arthur Naseef added a comment - Corrected patch - the original missed the all-important change to the PooledConnection.close() method itself. This was tested with the JUnit case provided and is working.
        Hide
        Arthur Naseef added a comment -

        JUnit test case which shows the temporary destinations of closed PooledConnection objects linger.

        Uses the destinationExists() method from the advisory/TempDestDeleteTest.java source - thanks!

        Show
        Arthur Naseef added a comment - JUnit test case which shows the temporary destinations of closed PooledConnection objects linger. Uses the destinationExists() method from the advisory/TempDestDeleteTest.java source - thanks!
        Hide
        Arthur Naseef added a comment -

        Producing to the temporary destination to test for its existence is not working. Any tips on how I can check if the temporary destination is deleted?

        Show
        Arthur Naseef added a comment - Producing to the temporary destination to test for its existence is not working. Any tips on how I can check if the temporary destination is deleted?
        Hide
        Arthur Naseef added a comment -

        Proposal for correcting the potential for lingering temporary destinations.

        This has not yet been tested; I'm posting it now to demonstrate my thinking. Unit test is being written now.

        Show
        Arthur Naseef added a comment - Proposal for correcting the potential for lingering temporary destinations. This has not yet been tested; I'm posting it now to demonstrate my thinking. Unit test is being written now.
        Hide
        Arthur Naseef added a comment -

        Below is my understanding. I'll work on a JUnit case, and send a proposal that I believe fixes the issue, a little later today.

        • The PooledConnection no longer cleans up the temp destinations itself
        • ConnectionPool removes the temp destinations after all references to the ConnectionPool are dropped (meaning that the underlying ActiveMQConnection is also no longer used)
        • PooledConnection gets its ConnectionPool from the PooledConnectionFactory when instantiated.
        • PooledConnectionFactory keeps a cache of ConnectionPool objects in a hash-map keyed on the user-name + passsword.
          • On createConnection(), if the ConnectionPool (with the matching username + password) is already in the cache, it is passed to the newly created PooledConnection.
          • If it's not already in the cache, a new one is added to the cache and used for the newly created PooledConnection.

        Here are the changes to ConnectionPool and PooledConnection for this bug that I believe back this up.

        Index: trunk/activemq-pool/src/main/java/org/apache/activemq/pool/ConnectionPool.java
        ===================================================================
        diff -u -N -r1071256 -r1158694
        --- trunk/activemq-pool/src/main/java/org/apache/activemq/pool/ConnectionPool.java	(.../ConnectionPool.java)	(revision 1071256)
        +++ trunk/activemq-pool/src/main/java/org/apache/activemq/pool/ConnectionPool.java	(.../ConnectionPool.java)	(revision 1158694)
        @@ -144,6 +144,12 @@
                 lastUsed = System.currentTimeMillis();
                 if (referenceCount == 0) {
                     expiredCheck();
        +            
        +            // only clean up temp destinations when all users 
        +            // of this connection have called close
        +            if (getConnection() != null) {
        +                getConnection().cleanUpTempDestinations();
        +            }
                 }
             }
         
        Index: trunk/activemq-pool/src/main/java/org/apache/activemq/pool/PooledConnection.java
        ===================================================================
        diff -u -N -r1142267 -r1158694
        --- trunk/activemq-pool/src/main/java/org/apache/activemq/pool/PooledConnection.java	(.../PooledConnection.java)	(revision 1142267)
        +++ trunk/activemq-pool/src/main/java/org/apache/activemq/pool/PooledConnection.java	(.../PooledConnection.java)	(revision 1158694)
        @@ -16,9 +16,6 @@
          */
         package org.apache.activemq.pool;
         
        -import java.util.Iterator;
        -import java.util.concurrent.ConcurrentHashMap;
        -
         import javax.jms.Connection;
         import javax.jms.ConnectionConsumer;
         import javax.jms.ConnectionMetaData;
        @@ -39,7 +36,6 @@
         import org.apache.activemq.AlreadyClosedException;
         import org.apache.activemq.EnhancedConnection;
         import org.apache.activemq.advisory.DestinationSource;
        -import org.apache.activemq.command.ActiveMQTempDestination;
         
         /**
          * Represents a proxy {@link Connection} which is-a {@link TopicConnection} and
        @@ -73,9 +69,6 @@
             public void close() throws JMSException {
                 if (this.pool != null) {
                     this.pool.decrementReferenceCount();
        -            if (this.pool.getConnection() != null) {
        -                this.pool.getConnection().cleanUpTempDestinations();
        -            }
                     this.pool = null;
                 }
             }
        

        By the way, for a JUnit test - do you have any recommendation on testing whether a temporary destination has been deleted? My plan is to produce to it from another connection and expect an exception.

        Show
        Arthur Naseef added a comment - Below is my understanding. I'll work on a JUnit case, and send a proposal that I believe fixes the issue, a little later today. The PooledConnection no longer cleans up the temp destinations itself ConnectionPool removes the temp destinations after all references to the ConnectionPool are dropped (meaning that the underlying ActiveMQConnection is also no longer used) PooledConnection gets its ConnectionPool from the PooledConnectionFactory when instantiated. PooledConnectionFactory keeps a cache of ConnectionPool objects in a hash-map keyed on the user-name + passsword. On createConnection(), if the ConnectionPool (with the matching username + password) is already in the cache, it is passed to the newly created PooledConnection. If it's not already in the cache, a new one is added to the cache and used for the newly created PooledConnection. Here are the changes to ConnectionPool and PooledConnection for this bug that I believe back this up. Index: trunk/activemq-pool/src/main/java/org/apache/activemq/pool/ConnectionPool.java =================================================================== diff -u -N -r1071256 -r1158694 --- trunk/activemq-pool/src/main/java/org/apache/activemq/pool/ConnectionPool.java (.../ConnectionPool.java) (revision 1071256) +++ trunk/activemq-pool/src/main/java/org/apache/activemq/pool/ConnectionPool.java (.../ConnectionPool.java) (revision 1158694) @@ -144,6 +144,12 @@ lastUsed = System .currentTimeMillis(); if (referenceCount == 0) { expiredCheck(); + + // only clean up temp destinations when all users + // of this connection have called close + if (getConnection() != null ) { + getConnection().cleanUpTempDestinations(); + } } } Index: trunk/activemq-pool/src/main/java/org/apache/activemq/pool/PooledConnection.java =================================================================== diff -u -N -r1142267 -r1158694 --- trunk/activemq-pool/src/main/java/org/apache/activemq/pool/PooledConnection.java (.../PooledConnection.java) (revision 1142267) +++ trunk/activemq-pool/src/main/java/org/apache/activemq/pool/PooledConnection.java (.../PooledConnection.java) (revision 1158694) @@ -16,9 +16,6 @@ */ package org.apache.activemq.pool; - import java.util.Iterator; - import java.util.concurrent.ConcurrentHashMap; - import javax.jms.Connection; import javax.jms.ConnectionConsumer; import javax.jms.ConnectionMetaData; @@ -39,7 +36,6 @@ import org.apache.activemq.AlreadyClosedException; import org.apache.activemq.EnhancedConnection; import org.apache.activemq.advisory.DestinationSource; - import org.apache.activemq.command.ActiveMQTempDestination; /** * Represents a proxy {@link Connection} which is-a {@link TopicConnection} and @@ -73,9 +69,6 @@ public void close() throws JMSException { if ( this .pool != null ) { this .pool.decrementReferenceCount(); - if ( this .pool.getConnection() != null ) { - this .pool.getConnection().cleanUpTempDestinations(); - } this .pool = null ; } } By the way, for a JUnit test - do you have any recommendation on testing whether a temporary destination has been deleted? My plan is to produce to it from another connection and expect an exception.
        Hide
        Timothy Bish added a comment -

        ActiveMQConnection keeps track of the temp destinations, and the patch causes the PooledConnection to ask the connection to deletes the temp destinations it created once the PooledConnection is closed, so no need to deplicate that logic in the PooledConnection.

        If you are still seeing an issue then a JUnit test case to demonstrate the problem would be appropriate.

        Show
        Timothy Bish added a comment - ActiveMQConnection keeps track of the temp destinations, and the patch causes the PooledConnection to ask the connection to deletes the temp destinations it created once the PooledConnection is closed, so no need to deplicate that logic in the PooledConnection. If you are still seeing an issue then a JUnit test case to demonstrate the problem would be appropriate.
        Hide
        Jonathan Anstey added a comment -

        Possibly. Though I'm just going on memory here - haven't looked at the latest code Are you seeing this behavior in your application?

        Show
        Jonathan Anstey added a comment - Possibly. Though I'm just going on memory here - haven't looked at the latest code Are you seeing this behavior in your application?
        Hide
        Arthur Naseef added a comment -

        Is this a complete fix, or will it lead to temporary destinations that linger on a heavily reused connection?

        Doesn't the PooledConnection need to keep track of the temporary destinations created by its sessions and specifically delete those on close()?

        Show
        Arthur Naseef added a comment - Is this a complete fix, or will it lead to temporary destinations that linger on a heavily reused connection? Doesn't the PooledConnection need to keep track of the temporary destinations created by its sessions and specifically delete those on close()?
        Hide
        Timothy Bish added a comment -
        Show
        Timothy Bish added a comment - Jon's fix was committed, see: http://svn.apache.org/viewvc?view=rev&rev=1179328
        Hide
        Edstrom Johan added a comment -

        To confirm the last comment, yup. You'll delete other clients destinations and in an active system this cleanup really will not hit...

        Show
        Edstrom Johan added a comment - To confirm the last comment, yup. You'll delete other clients destinations and in an active system this cleanup really will not hit...
        Hide
        Jonathan Anstey added a comment -

        Found another issue with this... so in the original bug multiple clients were using the same pooled connection and if one of those clients called close, it could potentially delete another client's temporary destination since they all shared the same connection. The fix was to wait until all clients had called close before deleting the temp destination.

        Now, in the case where there is a large pool of connections available, every client gets their own connection so the previous issue isn't a problem anymore. However, the AdvisoryConsumer holds on to a copy of all temp destinations and also adds each of these temp destinations to the connection it uses. So now, 2 connections have a reference to the same temp destination... which is a problem. If this connection is also used by a client and the client calls close, then the temp destinations will be deleted even though they are still used by another connection.

        Will commit a fix shortly for this.

        Show
        Jonathan Anstey added a comment - Found another issue with this... so in the original bug multiple clients were using the same pooled connection and if one of those clients called close, it could potentially delete another client's temporary destination since they all shared the same connection. The fix was to wait until all clients had called close before deleting the temp destination. Now, in the case where there is a large pool of connections available, every client gets their own connection so the previous issue isn't a problem anymore. However, the AdvisoryConsumer holds on to a copy of all temp destinations and also adds each of these temp destinations to the connection it uses. So now, 2 connections have a reference to the same temp destination... which is a problem. If this connection is also used by a client and the client calls close, then the temp destinations will be deleted even though they are still used by another connection. Will commit a fix shortly for this.
        Hide
        Jonathan Anstey added a comment -

        Awesome. Thanks for the quick review!

        Show
        Jonathan Anstey added a comment - Awesome. Thanks for the quick review!
        Hide
        Timothy Bish added a comment -

        Patch applied, thanks for catching that.

        Show
        Timothy Bish added a comment - Patch applied, thanks for catching that.

          People

          • Assignee:
            Jonathan Anstey
            Reporter:
            Jonathan Anstey
          • Votes:
            0 Vote for this issue
            Watchers:
            2 Start watching this issue

            Dates

            • Created:
              Updated:
              Resolved:

              Development