Fix for: https://issues.apache.org/jira/browse/AMQ-4366

git-svn-id: https://svn.apache.org/repos/asf/activemq/trunk@1454514 13f79535-47bb-0310-9956-ffa450edef68

activemq-pool/src/main/java/org/apache/activemq/pool/ConnectionKey.java

@@ -38,16 +38,18 @@ public class ConnectionKey {
         }
     }
 
+    @Override
     public int hashCode() {
         return hash;
     }
 
+    @Override
     public boolean equals(Object that) {
         if (this == that) {
             return true;
         }
         if (that instanceof ConnectionKey) {
-            return equals((ConnectionKey)that);
+            return equals((ConnectionKey) that);
         }
         return false;
     }
@@ -70,5 +72,4 @@ public class ConnectionKey {
         }
         return o1 != null && o2 != null && o1.equals(o2);
     }
-
 }

activemq-pool/src/main/java/org/apache/activemq/pool/ConnectionPool.java

@@ -46,7 +46,7 @@ public class ConnectionPool {
     private ActiveMQConnection connection;
     private int referenceCount;
     private long lastUsed = System.currentTimeMillis();
-    private long firstUsed = lastUsed;
+    private final long firstUsed = lastUsed;
     private boolean hasFailed;
     private boolean hasExpired;
     private int idleTimeout = 30 * 1000;
@@ -63,18 +63,22 @@ public class ConnectionPool {
         // Add a transport Listener so that we can notice if this connection
         // should be expired due to a connection failure.
         connection.addTransportListener(new TransportListener() {
+            @Override
             public void onCommand(Object command) {
             }
 
+            @Override
             public void onException(IOException error) {
                 synchronized (ConnectionPool.this) {
                     hasFailed = true;
                 }
             }
 
+            @Override
             public void transportInterupted() {
             }
 
+            @Override
             public void transportResumed() {
             }
         });
@@ -171,8 +175,6 @@ public class ConnectionPool {
         referenceCount--;
         lastUsed = System.currentTimeMillis();
         if (referenceCount == 0) {
-            expiredCheck();
-
             // Loaned sessions are those that are active in the sessionPool and
             // have not been closed by the client before closing the connection.
             // These need to be closed so that all session's reflect the fact
@@ -190,6 +192,8 @@ public class ConnectionPool {
             if (getConnection() != null) {
                 getConnection().cleanUpTempDestinations();
             }
+
+            expiredCheck();
         }
     }
 
@@ -207,23 +211,27 @@ public class ConnectionPool {
             return true;
         }
 
-        if (hasExpired) {
+        if (hasExpired || hasFailed) {
             if (referenceCount == 0) {
                 close();
             }
             return true;
         }
 
-        if (hasFailed
-                || (idleTimeout > 0 && System.currentTimeMillis() > lastUsed + idleTimeout)
-                || expiryTimeout > 0 && System.currentTimeMillis() > firstUsed + expiryTimeout) {
-
+        if (expiryTimeout > 0 && System.currentTimeMillis() > firstUsed + expiryTimeout) {
             hasExpired = true;
             if (referenceCount == 0) {
                 close();
             }
             return true;
         }
+
+        if (referenceCount == 0 && idleTimeout > 0 && System.currentTimeMillis() > lastUsed + idleTimeout) {
+            hasExpired = true;
+            close();
+            return true;
+        }
+
         return false;
     }
 

activemq-pool/src/main/java/org/apache/activemq/pool/PooledConnection.java

@@ -142,7 +142,6 @@ public class PooledConnection implements TopicConnection, QueueConnection, Enhan
 
     @Override
     public void setClientID(String clientID) throws JMSException {
-
         // ignore repeated calls to setClientID() with the same client id
         // this could happen when a JMS component such as Spring that uses a
         // PooledConnectionFactory shuts down and reinitializes.

activemq-pool/src/main/java/org/apache/activemq/pool/PooledConnectionFactory.java

@@ -359,21 +359,21 @@ public class PooledConnectionFactory implements ConnectionFactory, Service {
      * The idle timeout is used determine if a Connection instance has sat to long in the pool unused
      * and if so is closed and removed from the pool.  The default value is 30 seconds.
      *
-     * @return
+     * @return idle timeout value (milliseconds)
      */
     public int getIdleTimeout() {
         return idleTimeout;
     }
 
     /**
-     * Sets the idle timeout value for Connection's that are created by this pool, defaults to 30 seconds.
+     * Sets the idle timeout  value for Connection's that are created by this pool in Milliseconds,
+     * defaults to 30 seconds.
      * <p/>
      * For a Connection that is in the pool but has no current users the idle timeout determines how
      * long the Connection can live before it is eligible for removal from the pool.  Normally the
      * connections are tested when an attempt to check one out occurs so a Connection instance can sit
      * in the pool much longer than its idle timeout if connections are used infrequently.
      *
-     *
      * @param idleTimeout
      *      The maximum time a pooled Connection can sit unused before it is eligible for removal.
      */

activemq-pool/src/test/java/org/apache/activemq/pool/ConnectionExpiryEvictsFromPoolTest.java

@@ -19,6 +19,7 @@ package org.apache.activemq.pool;
 import java.util.concurrent.TimeUnit;
 
 import javax.jms.Connection;
+import javax.jms.Session;
 
 import org.apache.activemq.ActiveMQConnection;
 import org.apache.activemq.ActiveMQConnectionFactory;
@@ -32,6 +33,7 @@ public class ConnectionExpiryEvictsFromPoolTest extends TestSupport {
     private ActiveMQConnectionFactory factory;
     private PooledConnectionFactory pooledFactory;
 
+    @Override
     protected void setUp() throws Exception {
         broker = new BrokerService();
         broker.setUseJmx(false);
@@ -57,7 +59,6 @@ public class ConnectionExpiryEvictsFromPoolTest extends TestSupport {
         assertTrue("not equal", !amq1.equals(amq2));
     }
 
-
     public void testEvictionOfExpired() throws Exception {
         pooledFactory.setExpiryTimeout(10);
         Connection connection = pooledFactory.createConnection();
@@ -72,7 +73,29 @@ public class ConnectionExpiryEvictsFromPoolTest extends TestSupport {
         assertTrue("not equal", !amq1.equals(amq2));
     }
 
+    public void testRetainIdleWhenInUse() throws Exception {
+        pooledFactory.setIdleTimeout(10);
+        PooledConnection connection = (PooledConnection) pooledFactory.createConnection();
+        Session s = connection.createSession(false, Session.AUTO_ACKNOWLEDGE);
 
+        // let connection to get idle
+        TimeUnit.SECONDS.sleep(1);
+
+        // get the same connection from pool again, it will get destroyed due to validation check
+        // it will be the same since maxIdle is set to 1 in implementation
+        PooledConnection connection2 = (PooledConnection) pooledFactory.createConnection();
+        assertSame(connection.getConnection(), connection2.getConnection());
+
+        // now the session is closed even when it should not be
+        try {
+            // any operation on session first checks whether session is closed
+            s.getTransacted();
+        } catch (javax.jms.IllegalStateException e) {
+            assertTrue("Session should be fine, instead: " + e.getMessage(), false);
+        }
+    }
+
+    @Override
     protected void tearDown() throws Exception {
         broker.stop();
     }