QPID-1126: reuse channel numbers for sessions that have closed, and honor the negotiated channel-max; also removed unnecessary catches that were swallowing stack traces from several tests

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

9 files changed, 64 insertions, 174 deletions

diff --git a/java/client/src/main/java/org/apache/qpidity/nclient/Client.java b/java/client/src/main/java/org/apache/qpidity/nclient/Client.java
index 9fb4c541a9..bc88160137 100644
--- a/java/client/src/main/java/org/apache/qpidity/nclient/Client.java
+++ b/java/client/src/main/java/org/apache/qpidity/nclient/Client.java
@@ -22,7 +22,6 @@ package org.apache.qpidity.nclient;
 import java.util.List;
 import java.util.UUID;
 import java.util.concurrent.TimeUnit;
-import java.util.concurrent.atomic.AtomicInteger;
 import java.util.concurrent.locks.Condition;
 import java.util.concurrent.locks.Lock;
 import java.util.concurrent.locks.ReentrantLock;
@@ -55,7 +54,6 @@ import org.slf4j.LoggerFactory;
 
 public class Client implements org.apache.qpidity.nclient.Connection
 {
-    private AtomicInteger _channelNo = new AtomicInteger();
     private Connection _conn;
     private ClosedListener _closedListner;
     private final Lock _lock = new ReentrantLock();
@@ -286,7 +284,7 @@ public class Client implements org.apache.qpidity.nclient.Connection
 
     public Session createSession(long expiryInSeconds)
     {
-        Channel ch = _conn.getChannel(_channelNo.incrementAndGet());
+        Channel ch = _conn.getChannel();
         ClientSession ssn = new ClientSession(UUID.randomUUID().toString().getBytes());
         ssn.attach(ch);
         ssn.sessionAttach(ssn.getName());
diff --git a/java/client/src/test/java/org/apache/qpid/test/unit/xa/AbstractXATestCase.java b/java/client/src/test/java/org/apache/qpid/test/unit/xa/AbstractXATestCase.java
index c8ec62d059..18cdb645c6 100644
--- a/java/client/src/test/java/org/apache/qpid/test/unit/xa/AbstractXATestCase.java
+++ b/java/client/src/test/java/org/apache/qpid/test/unit/xa/AbstractXATestCase.java
@@ -65,7 +65,7 @@ public abstract  class AbstractXATestCase extends QpidTestCase
         init();
     }
 
-    public abstract  void init();
+    public abstract  void init() throws Exception;
 
 
     
diff --git a/java/client/src/test/java/org/apache/qpid/test/unit/xa/FaultTest.java b/java/client/src/test/java/org/apache/qpid/test/unit/xa/FaultTest.java
index 8f291897eb..0adf39980b 100644
--- a/java/client/src/test/java/org/apache/qpid/test/unit/xa/FaultTest.java
+++ b/java/client/src/test/java/org/apache/qpid/test/unit/xa/FaultTest.java
@@ -75,15 +75,8 @@ public class FaultTest extends AbstractXATestCase
     {
         if (!isBroker08())
         {
-            try
-            {
-                _xaqueueConnection.close();
-                _queueConnection.close();
-            }
-            catch (Exception e)
-            {
-                fail("Exception thrown when cleaning standard connection: " + e);
-            }
+            _xaqueueConnection.close();
+            _queueConnection.close();
         }
         super.tearDown();
     }
@@ -91,57 +84,16 @@ public class FaultTest extends AbstractXATestCase
     /**
      * Initialize standard actors
      */
-    public void init()
+    public void init() throws Exception
     {
         if (!isBroker08())
         {
-            // lookup test queue
-            try
-            {
-                _queue = (Queue) getInitialContext().lookup(QUEUENAME);
-            }
-            catch (Exception e)
-            {
-                fail("cannot lookup test queue " + e.getMessage());
-            }
-            // lookup connection factory
-            try
-            {
-                _queueFactory = getConnectionFactory();
-            }
-            catch (Exception e)
-            {
-                fail("enable to lookup connection factory ");
-            }
-            // create standard connection
-            try
-            {
-                _xaqueueConnection = _queueFactory.createXAQueueConnection("guest", "guest");
-            }
-            catch (JMSException e)
-            {
-                fail("cannot create queue connection: " + e.getMessage());
-            }
-            // create xa session
-            XAQueueSession session = null;
-            try
-            {
-                session = _xaqueueConnection.createXAQueueSession();
-            }
-            catch (JMSException e)
-            {
-                fail("cannot create queue session: " + e.getMessage());
-            }
-            // create a standard session
-            try
-            {
-                _queueConnection = _queueFactory.createQueueConnection();
-                _nonXASession = _queueConnection.createQueueSession(true, Session.AUTO_ACKNOWLEDGE);
-            }
-            catch (JMSException e)
-            {
-                fail("cannot create queue session: " + e.getMessage());
-            }
+            _queue = (Queue) getInitialContext().lookup(QUEUENAME);
+            _queueFactory = getConnectionFactory();
+            _xaqueueConnection = _queueFactory.createXAQueueConnection("guest", "guest");
+            XAQueueSession session = _xaqueueConnection.createXAQueueSession();
+            _queueConnection = _queueFactory.createQueueConnection();
+            _nonXASession = _queueConnection.createQueueSession(true, Session.AUTO_ACKNOWLEDGE);
             init(session, _queue);
         }
     }
@@ -156,18 +108,10 @@ public class FaultTest extends AbstractXATestCase
      * Check that the second
      * invocation is throwing the expected XA exception.
      */
-    public void testSameXID()
+    public void testSameXID() throws Exception
     {
-        _logger.debug("running testSameXID");
         Xid xid = getNewXid();
-        try
-        {
-            _xaResource.start(xid, XAResource.TMNOFLAGS);
-        }
-        catch (XAException e)
-        {
-            fail("cannot start the transaction with xid: " + e.getMessage());
-        }
+        _xaResource.start(xid, XAResource.TMNOFLAGS);
         // we now exepct this operation to fail
         try
         {
@@ -178,10 +122,6 @@ public class FaultTest extends AbstractXATestCase
         {
             assertEquals("Wrong error code: ", XAException.XAER_DUPID, e.errorCode);
         }
-        catch (Exception ex)
-        {
-            fail("Caught wrong exception, expected XAException, got: " + ex);
-        }
     }
 
     /**
@@ -191,7 +131,6 @@ public class FaultTest extends AbstractXATestCase
      */
     public void testWrongStartFlag()
     {
-        _logger.debug("running testWrongStartFlag");
         Xid xid = getNewXid();
         try
         {
@@ -202,10 +141,6 @@ public class FaultTest extends AbstractXATestCase
         {
             assertEquals("Wrong error code: ", XAException.XAER_INVAL, e.errorCode);
         }
-        catch (Exception ex)
-        {
-            fail("Caught wrong exception, expected XAException, got: " + ex);
-        }
     }
 
     /**
@@ -215,7 +150,6 @@ public class FaultTest extends AbstractXATestCase
      */
     public void testEnd()
     {
-        _logger.debug("running testEnd");
         Xid xid = getNewXid();
         try
         {
@@ -226,10 +160,6 @@ public class FaultTest extends AbstractXATestCase
         {
             assertEquals("Wrong error code: ", XAException.XAER_PROTO, e.errorCode);
         }
-        catch (Exception ex)
-        {
-            fail("Caught wrong exception, expected XAException, got: " + ex);
-        }
     }
 
 
@@ -243,7 +173,6 @@ public class FaultTest extends AbstractXATestCase
      */
     public void testForget()
     {
-        _logger.debug("running testForget");
         Xid xid = getNewXid();
         try
         {
@@ -254,10 +183,6 @@ public class FaultTest extends AbstractXATestCase
         {
             // assertEquals("Wrong error code: ", XAException.XAER_NOTA, e.errorCode);
         }
-        catch (Exception ex)
-        {
-            fail("Caught wrong exception, expected XAException, got: " + ex);
-        }
         xid = getNewXid();
         try
         {
@@ -269,10 +194,6 @@ public class FaultTest extends AbstractXATestCase
         {
             assertEquals("Wrong error code: ", XAException.XAER_PROTO, e.errorCode);
         }
-        catch (Exception ex)
-        {
-            fail("Caught wrong exception, expected XAException, got: " + ex);
-        }
     }
 
     /**
@@ -283,7 +204,6 @@ public class FaultTest extends AbstractXATestCase
      */
     public void testPrepare()
     {
-        _logger.debug("running testPrepare");
         Xid xid = getNewXid();
         try
         {
@@ -294,10 +214,6 @@ public class FaultTest extends AbstractXATestCase
         {
             assertEquals("Wrong error code: ", XAException.XAER_NOTA, e.errorCode);
         }
-        catch (Exception ex)
-        {
-            fail("Caught wrong exception, expected XAException, got: " + ex);
-        }
         xid = getNewXid();
         try
         {
@@ -309,10 +225,6 @@ public class FaultTest extends AbstractXATestCase
         {
             assertEquals("Wrong error code: ", XAException.XAER_PROTO, e.errorCode);
         }
-        catch (Exception ex)
-        {
-            fail("Caught wrong exception, expected XAException, got: " + ex);
-        }
     }
 
     /**
@@ -323,9 +235,8 @@ public class FaultTest extends AbstractXATestCase
      * A non prepared xid is committed with one phase set to false.
      * A prepared xid is committed with one phase set to true.
      */
-    public void testCommit()
+    public void testCommit() throws Exception
     {
-        _logger.debug("running testCommit");
         Xid xid = getNewXid();
         try
         {
@@ -336,10 +247,6 @@ public class FaultTest extends AbstractXATestCase
         {
             assertEquals("Wrong error code: ", XAException.XAER_NOTA, e.errorCode);
         }
-        catch (Exception ex)
-        {
-            fail("Caught wrong exception, expected XAException, got: " + ex);
-        }
         xid = getNewXid();
         try
         {
@@ -351,10 +258,6 @@ public class FaultTest extends AbstractXATestCase
         {
             assertEquals("Wrong error code: ", XAException.XAER_PROTO, e.errorCode);
         }
-        catch (Exception ex)
-        {
-            fail("Caught wrong exception, expected XAException, got: " + ex);
-        }
         xid = getNewXid();
         try
         {
@@ -367,10 +270,6 @@ public class FaultTest extends AbstractXATestCase
         {
             assertEquals("Wrong error code: ", XAException.XAER_PROTO, e.errorCode);
         }
-        catch (Exception ex)
-        {
-            fail("Caught wrong exception, expected XAException, got: " + ex);
-        }
         xid = getNewXid();
         try
         {
@@ -384,20 +283,9 @@ public class FaultTest extends AbstractXATestCase
         {
             assertEquals("Wrong error code: ", XAException.XAER_PROTO, e.errorCode);
         }
-        catch (Exception ex)
-        {
-            fail("Caught wrong exception, expected XAException, got: " + ex);
-        }
         finally
         {
-            try
-            {
-                _xaResource.commit(xid, false);
-            }
-            catch (XAException e)
-            {
-               fail("Cannot commit prepared tx: " + e);
-            }
+            _xaResource.commit(xid, false);
         }
     }
 
@@ -409,7 +297,6 @@ public class FaultTest extends AbstractXATestCase
      */
     public void testRollback()
     {
-        _logger.debug("running testRollback");
         Xid xid = getNewXid();
         try
         {
@@ -420,10 +307,6 @@ public class FaultTest extends AbstractXATestCase
         {
             assertEquals("Wrong error code: ", XAException.XAER_NOTA, e.errorCode);
         }
-        catch (Exception ex)
-        {
-            fail("Caught wrong exception, expected XAException, got: " + ex);
-        }
         xid = getNewXid();
         try
         {
@@ -435,35 +318,23 @@ public class FaultTest extends AbstractXATestCase
         {
             assertEquals("Wrong error code: ", XAException.XAER_PROTO, e.errorCode);
         }
-        catch (Exception ex)
-        {
-            fail("Caught wrong exception, expected XAException, got: " + ex);
-        }
     }
 
     /**
      * Strategy:
      * Check that the timeout is set correctly
      */
-    public void testTransactionTimeoutvalue()
+    public void testTransactionTimeoutvalue() throws Exception
     {
-        _logger.debug("running testRollback");
         Xid xid = getNewXid();
-        try
-        {
-            _xaResource.start(xid, XAResource.TMNOFLAGS);
-            assertEquals("Wrong timeout", _xaResource.getTransactionTimeout(), 0);
-            _xaResource.setTransactionTimeout(1000);
-            assertEquals("Wrong timeout", _xaResource.getTransactionTimeout(), 1000);
-            _xaResource.end(xid, XAResource.TMSUCCESS);
-            xid = getNewXid();
-            _xaResource.start(xid, XAResource.TMNOFLAGS);
-            assertEquals("Wrong timeout", _xaResource.getTransactionTimeout(), 0);            
-        }
-        catch (Exception ex)
-        {
-            fail("Caught wrong exception, expected XAException, got: " + ex);
-        }
+        _xaResource.start(xid, XAResource.TMNOFLAGS);
+        assertEquals("Wrong timeout", _xaResource.getTransactionTimeout(), 0);
+        _xaResource.setTransactionTimeout(1000);
+        assertEquals("Wrong timeout", _xaResource.getTransactionTimeout(), 1000);
+        _xaResource.end(xid, XAResource.TMSUCCESS);
+        xid = getNewXid();
+        _xaResource.start(xid, XAResource.TMNOFLAGS);
+        assertEquals("Wrong timeout", _xaResource.getTransactionTimeout(), 0);
     }
 
     /**
@@ -471,11 +342,10 @@ public class FaultTest extends AbstractXATestCase
      * Check that a transaction timeout as expected
      * - set timeout to 10ms
      * - sleep 1000ms
-     * - call end and check that the expected exception is thrown   
+     * - call end and check that the expected exception is thrown
      */
-    public void testTransactionTimeout()
+    public void testTransactionTimeout() throws Exception
     {
-        _logger.debug("running testRollback");
         Xid xid = getNewXid();
         try
         {
@@ -489,9 +359,6 @@ public class FaultTest extends AbstractXATestCase
         {
             assertEquals("Wrong error code: ", XAException.XA_RBTIMEOUT, e.errorCode);
         }
-        catch (Exception ex)
-        {
-            fail("Caught wrong exception, expected XAException, got: " + ex);
-        }
     }
+
 }
diff --git a/java/common/src/main/java/org/apache/qpidity/transport/Channel.java b/java/common/src/main/java/org/apache/qpidity/transport/Channel.java
index ad727676c4..2e11329c5b 100644
--- a/java/common/src/main/java/org/apache/qpidity/transport/Channel.java
+++ b/java/common/src/main/java/org/apache/qpidity/transport/Channel.java
@@ -131,10 +131,6 @@ public class Channel extends Invoker
         {
             session.closed();
         }
-    }
-
-    public void close()
-    {
         connection.removeChannel(channel);
     }
 
diff --git a/java/common/src/main/java/org/apache/qpidity/transport/ChannelDelegate.java b/java/common/src/main/java/org/apache/qpidity/transport/ChannelDelegate.java
index 9470520937..96578ffeb8 100644
--- a/java/common/src/main/java/org/apache/qpidity/transport/ChannelDelegate.java
+++ b/java/common/src/main/java/org/apache/qpidity/transport/ChannelDelegate.java
@@ -41,12 +41,14 @@ class ChannelDelegate extends MethodDelegate<Channel>
 
     public @Override void sessionDetached(Channel channel, SessionDetached closed)
     {
-        channel.getSession().closed();
+        channel.closed();
     }
 
     public @Override void sessionDetach(Channel channel, SessionDetach dtc)
     {
         channel.getSession().closed();
+        channel.sessionDetached(dtc.getName(), SessionDetachCode.NORMAL);
+        channel.closed();
     }
 
 }
diff --git a/java/common/src/main/java/org/apache/qpidity/transport/Connection.java b/java/common/src/main/java/org/apache/qpidity/transport/Connection.java
index 7d707ce17b..9829343491 100644
--- a/java/common/src/main/java/org/apache/qpidity/transport/Connection.java
+++ b/java/common/src/main/java/org/apache/qpidity/transport/Connection.java
@@ -22,8 +22,9 @@ package org.apache.qpidity.transport;
 
 import org.apache.qpidity.transport.util.Logger;
 
+import java.util.ArrayList;
 import java.util.HashMap;
-import java.util.Iterator;
+import java.util.List;
 import java.util.Map;
 
 import java.nio.ByteBuffer;
@@ -48,6 +49,7 @@ public class Connection
 
     final private Sender<ConnectionEvent> sender;
     final private ConnectionDelegate delegate;
+    private int channelMax = 1;
     // want to make this final
     private int _connectionId;
 
@@ -88,6 +90,32 @@ public class Connection
         sender.send(event);
     }
 
+    public int getChannelMax()
+    {
+        return channelMax;
+    }
+
+    void setChannelMax(int max)
+    {
+        channelMax = max;
+    }
+
+    public Channel getChannel()
+    {
+        synchronized (channels)
+        {
+            for (int i = 0; i < getChannelMax(); i++)
+            {
+                if (!channels.containsKey(i))
+                {
+                    return getChannel(i);
+                }
+            }
+
+            throw new RuntimeException("no more channels available");
+        }
+    }
+
     public Channel getChannel(int number)
     {
         synchronized (channels)
@@ -120,11 +148,10 @@ public class Connection
         log.debug("connection closed: %s", this);
         synchronized (channels)
         {
-            for (Iterator<Channel> it = channels.values().iterator();
-                 it.hasNext(); )
+            List<Channel> values = new ArrayList<Channel>(channels.values());
+            for (Channel ch : values)
             {
-                it.next().closed();
-                it.remove();
+                ch.closed();
             }
         }
         delegate.closed();
diff --git a/java/common/src/main/java/org/apache/qpidity/transport/ConnectionDelegate.java b/java/common/src/main/java/org/apache/qpidity/transport/ConnectionDelegate.java
index cb5f05a185..14344991c6 100644
--- a/java/common/src/main/java/org/apache/qpidity/transport/ConnectionDelegate.java
+++ b/java/common/src/main/java/org/apache/qpidity/transport/ConnectionDelegate.java
@@ -152,7 +152,7 @@ public abstract class ConnectionDelegate extends MethodDelegate<Channel>
 
     @Override public void connectionTune(Channel context, ConnectionTune struct)
     {
-        // should update the channel max given by the broker.
+        context.getConnection().setChannelMax(struct.getChannelMax());
         context.connectionTuneOk(struct.getChannelMax(), struct.getMaxFrameSize(), struct.getHeartbeatMax());
         context.connectionOpen(_virtualHost, null, Option.INSIST);
     }
diff --git a/java/common/src/main/java/org/apache/qpidity/transport/Session.java b/java/common/src/main/java/org/apache/qpidity/transport/Session.java
index d1ea23035a..a0229adf1e 100644
--- a/java/common/src/main/java/org/apache/qpidity/transport/Session.java
+++ b/java/common/src/main/java/org/apache/qpidity/transport/Session.java
@@ -526,7 +526,6 @@ public class Session extends Invoker
                 }
             }
         }
-        channel.close();
         channel.setSession(null);
         channel = null;
     }
diff --git a/java/systests/src/main/java/org/apache/qpid/server/security/acl/SimpleACLTest.java b/java/systests/src/main/java/org/apache/qpid/server/security/acl/SimpleACLTest.java
index 9ba0f6024c..e39b1122cf 100644
--- a/java/systests/src/main/java/org/apache/qpid/server/security/acl/SimpleACLTest.java
+++ b/java/systests/src/main/java/org/apache/qpid/server/security/acl/SimpleACLTest.java
@@ -575,6 +575,7 @@ public class SimpleACLTest extends TestCase implements ConnectionListener
         catch (JMSException e)
         {
             Throwable cause = e.getLinkedException();
+            cause.printStackTrace();
             assertEquals("Incorrect exception", AMQAuthenticationException.class, cause.getClass());
             assertEquals("Incorrect error code thrown", 403, ((AMQAuthenticationException) cause).getErrorCode().getCode());
         }