QPID-940,QPID-594,QPID-805,QPID-826 : Updated the client exception handling so that exceptions are not lost. In performing the changes I noted that the AMQStateManager is only used for connection creation in the 08/09 codepath. Now this may be due to the fact that we don't currently need to wait on any other states. We need to improve our testing of error conditions for all protcol versions.

Changes Summary:

The MethodHandlers had their AMQStateManager parameters swapped for AMQSession as that is what they all cared about.
The BlockingMethodFrameListener was used as a basis to create a generic BlockingWaiter which is now used by StateWaiter,
There is probably scope to refactor the AMQStateManager and the parts of the AMQProtocolHandler that deal with the _frameListeners into a generic class but that can be looked at as part of a wider client refactor.

Additionally tests were updated such as SimpleACLTest and ConnectionTest as they were expecting JMSExceptions from the constructor but the JMS API does not demand that and AMQExceptions are now correctly being thrown.

The SimpleACLTest also required a change to AMQSession.
The test calls send which will cause the connection to be closed asynchrously due to a permission violation. As this exception is not expected and is asynchorous to the client code it cannot be directly thrown. The solution is to record this exception in the AMQStateManager, it can tell that there are no waiters for the exception so it can record the value.(Potential exists to alert if the exception is overwritten but I don't think this is required right now)

When the AMQSession checks that the connection is closed it is then possible to check if the current State is CLOSED and if we have an exception sitting in the AMQStateManager. If all these are true we can attach the AMQStateManager exception to the IllegalState Exception that is normally thrown when the Session is closed.

This maintains JMS Compliance and allows us to discover the cause of the failure, SimpleACLTest was updated by removing the IllegalState catch section that was causing the test to silently fail.


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

5 files changed, 91 insertions, 109 deletions

diff --git a/qpid/java/systests/build.xml b/qpid/java/systests/build.xml
index f508fec606..4eb7275e73 100644
--- a/qpid/java/systests/build.xml
+++ b/qpid/java/systests/build.xml
@@ -23,7 +23,7 @@ nn - or more contributor license agreements.  See the NOTICE file
     <property name="module.depends" value="client broker common junit-toolkit"/>
     <property name="module.test.src" location="src/main/java"/>
     <property name="module.test.excludes"
-              value="**/TTLTest.java,**/DropInTest.java,**/TestClientControlledTest.java,**/SimpleACLTest.java"/>
+              value="**/TTLTest.java,**/DropInTest.java,**/TestClientControlledTest.java"/>
 
     <import file="../module.xml"/>
 
diff --git a/qpid/java/systests/src/main/java/org/apache/qpid/server/exchange/ReturnUnroutableMandatoryMessageTest.java b/qpid/java/systests/src/main/java/org/apache/qpid/server/exchange/ReturnUnroutableMandatoryMessageTest.java
index bae3f844d7..40d6f35c76 100644
--- a/qpid/java/systests/src/main/java/org/apache/qpid/server/exchange/ReturnUnroutableMandatoryMessageTest.java
+++ b/qpid/java/systests/src/main/java/org/apache/qpid/server/exchange/ReturnUnroutableMandatoryMessageTest.java
@@ -29,13 +29,16 @@ import org.apache.qpid.client.*;
 import org.apache.qpid.client.transport.TransportConnection;
 import org.apache.qpid.url.AMQBindingURL;
 import org.apache.qpid.url.BindingURL;
+import org.apache.qpid.url.URLSyntaxException;
 import org.apache.qpid.exchange.ExchangeDefaults;
 import org.apache.qpid.framing.FieldTable;
+import org.apache.qpid.AMQException;
 
 import javax.jms.*;
 import java.util.List;
 import java.util.Collections;
 import java.util.ArrayList;
+import java.net.URISyntaxException;
 
 public class ReturnUnroutableMandatoryMessageTest extends TestCase implements ExceptionListener
 {
@@ -75,78 +78,90 @@ public class ReturnUnroutableMandatoryMessageTest extends TestCase implements Ex
      *
      * @throws Exception
      */
-    public void testReturnUnroutableMandatoryMessage_HEADERS() throws Exception
+    public void testReturnUnroutableMandatoryMessage_HEADERS() throws URISyntaxException, AMQException, JMSException
     {
         _bouncedMessageList.clear();
-        Connection con = new AMQConnection(BROKER, "guest", "guest", "consumer1", VIRTUALHOST);
-
-
-        AMQSession consumerSession = (AMQSession) con.createSession(false, Session.CLIENT_ACKNOWLEDGE);
-
-        AMQHeadersExchange queue = new AMQHeadersExchange(new AMQBindingURL(ExchangeDefaults.HEADERS_EXCHANGE_CLASS + "://" + ExchangeDefaults.HEADERS_EXCHANGE_NAME + "/test/queue1?" + BindingURL.OPTION_ROUTING_KEY + "='F0000=1'"));
-        FieldTable ft = new FieldTable();
-        ft.setString("F1000", "1");
-        MessageConsumer consumer = consumerSession.createConsumer(queue, AMQSession.DEFAULT_PREFETCH_LOW_MARK, AMQSession.DEFAULT_PREFETCH_HIGH_MARK, false, false, (String) null, ft);
-
-        //force synch to ensure the consumer has resulted in a bound queue
-        //((AMQSession) consumerSession).declareExchangeSynch(ExchangeDefaults.HEADERS_EXCHANGE_NAME, ExchangeDefaults.HEADERS_EXCHANGE_CLASS);
-        // This is the default now
-
-        Connection con2 = new AMQConnection(BROKER, "guest", "guest", "producer1", VIRTUALHOST);
-
-        con2.setExceptionListener(this);
-        AMQSession producerSession = (AMQSession) con2.createSession(false, Session.CLIENT_ACKNOWLEDGE);
-
-        // Need to start the "producer" connection in order to receive bounced messages
-        _logger.info("Starting producer connection");
-        con2.start();
-
+        MessageConsumer consumer = null;
+        AMQSession producerSession = null;
+        AMQHeadersExchange queue = null;
+        Connection con=null, con2 = null;
+        try
+        {
+            con = new AMQConnection(BROKER, "guest", "guest", "consumer1", VIRTUALHOST);
 
-        MessageProducer nonMandatoryProducer = producerSession.createProducer(queue, false, false);
-        MessageProducer mandatoryProducer = producerSession.createProducer(queue);
+            AMQSession consumerSession = (AMQSession) con.createSession(false, Session.CLIENT_ACKNOWLEDGE);
 
-        // First test - should neither be bounced nor routed
-        _logger.info("Sending non-routable non-mandatory message");
-        TextMessage msg1 = producerSession.createTextMessage("msg1");
-        nonMandatoryProducer.send(msg1);
+            queue = new AMQHeadersExchange(new AMQBindingURL(ExchangeDefaults.HEADERS_EXCHANGE_CLASS + "://" + ExchangeDefaults.HEADERS_EXCHANGE_NAME + "/test/queue1?" + BindingURL.OPTION_ROUTING_KEY + "='F0000=1'"));
+            FieldTable ft = new FieldTable();
+            ft.setString("F1000", "1");
+            consumer = consumerSession.createConsumer(queue, AMQSession.DEFAULT_PREFETCH_LOW_MARK, AMQSession.DEFAULT_PREFETCH_HIGH_MARK, false, false, (String) null, ft);
 
-        // Second test - should be bounced
-        _logger.info("Sending non-routable mandatory message");
-        TextMessage msg2 = producerSession.createTextMessage("msg2");
-        mandatoryProducer.send(msg2);
+            //force synch to ensure the consumer has resulted in a bound queue
+            //((AMQSession) consumerSession).declareExchangeSynch(ExchangeDefaults.HEADERS_EXCHANGE_NAME, ExchangeDefaults.HEADERS_EXCHANGE_CLASS);
+            // This is the default now
 
-        // Third test - should be routed
-        _logger.info("Sending routable message");
-        TextMessage msg3 = producerSession.createTextMessage("msg3");
-        msg3.setStringProperty("F1000", "1");
-        mandatoryProducer.send(msg3);
+            con2 = new AMQConnection(BROKER, "guest", "guest", "producer1", VIRTUALHOST);
 
+            con2.setExceptionListener(this);
+            producerSession = (AMQSession) con2.createSession(false, Session.CLIENT_ACKNOWLEDGE);
 
-        _logger.info("Starting consumer connection");
-        con.start();
-        TextMessage tm = (TextMessage) consumer.receive(1000L);
-
-        assertTrue("No message routed to receiver", tm != null);
-        assertTrue("Wrong message routed to receiver: " + tm.getText(), "msg3".equals(tm.getText()));
+            // Need to start the "producer" connection in order to receive bounced messages
+            _logger.info("Starting producer connection");
+            con2.start();
+        }
+        catch (JMSException jmse)
+        {
+            fail(jmse.getMessage());
+        }
 
         try
         {
-            Thread.sleep(1000L);
+            MessageProducer nonMandatoryProducer = producerSession.createProducer(queue, false, false);
+            MessageProducer mandatoryProducer = producerSession.createProducer(queue);
+
+            // First test - should neither be bounced nor routed
+            _logger.info("Sending non-routable non-mandatory message");
+            TextMessage msg1 = producerSession.createTextMessage("msg1");
+            nonMandatoryProducer.send(msg1);
+
+            // Second test - should be bounced
+            _logger.info("Sending non-routable mandatory message");
+            TextMessage msg2 = producerSession.createTextMessage("msg2");
+            mandatoryProducer.send(msg2);
+
+            // Third test - should be routed
+            _logger.info("Sending routable message");
+            TextMessage msg3 = producerSession.createTextMessage("msg3");
+            msg3.setStringProperty("F1000", "1");
+            mandatoryProducer.send(msg3);
+
+            _logger.info("Starting consumer connection");
+            con.start();
+            TextMessage tm = (TextMessage) consumer.receive(1000L);
+
+            assertTrue("No message routed to receiver", tm != null);
+            assertTrue("Wrong message routed to receiver: " + tm.getText(), "msg3".equals(tm.getText()));
+
+            try
+            {
+                Thread.sleep(1000L);
+            }
+            catch (InterruptedException e)
+            {
+                ;
+            }
+
+            assertTrue("Wrong number of messages bounced (expect 1): " + _bouncedMessageList.size(), _bouncedMessageList.size() == 1);
+            Message m = _bouncedMessageList.get(0);
+            assertTrue("Wrong message bounced: " + m.toString(), m.toString().contains("msg2"));
         }
-        catch (InterruptedException e)
+        catch (JMSException jmse)
         {
-            ;
-        }
-
-        assertTrue("Wrong number of messages bounced (expect 1): " + _bouncedMessageList.size(), _bouncedMessageList.size() == 1);
-        Message m = _bouncedMessageList.get(0);
-        assertTrue("Wrong message bounced: " + m.toString(), m.toString().contains("msg2"));
-
 
+        }
         con.close();
         con2.close();
 
-
     }
 
     public void testReturnUnroutableMandatoryMessage_QUEUE() throws Exception
@@ -154,7 +169,6 @@ public class ReturnUnroutableMandatoryMessageTest extends TestCase implements Ex
         _bouncedMessageList.clear();
         Connection con = new AMQConnection(BROKER, "guest", "guest", "consumer1", VIRTUALHOST);
 
-
         AMQSession consumerSession = (AMQSession) con.createSession(false, Session.CLIENT_ACKNOWLEDGE);
 
         AMQQueue valid_queue = new AMQQueue(ExchangeDefaults.DIRECT_EXCHANGE_CLASS, "testReturnUnroutableMandatoryMessage_QUEUE");
@@ -174,7 +188,6 @@ public class ReturnUnroutableMandatoryMessageTest extends TestCase implements Ex
         _logger.info("Starting producer connection");
         con2.start();
 
-
         MessageProducer nonMandatoryProducer = producerSession.createProducer(valid_queue, false, false);
         MessageProducer mandatoryProducer = producerSession.createProducer(invalid_queue);
 
@@ -188,7 +201,6 @@ public class ReturnUnroutableMandatoryMessageTest extends TestCase implements Ex
         TextMessage msg2 = producerSession.createTextMessage("msg2");
         mandatoryProducer.send(msg2);
 
-
         _logger.info("Starting consumer connection");
         con.start();
         TextMessage tm = (TextMessage) consumer.receive(1000L);
@@ -209,18 +221,15 @@ public class ReturnUnroutableMandatoryMessageTest extends TestCase implements Ex
         Message m = _bouncedMessageList.get(0);
         assertTrue("Wrong message bounced: " + m.toString(), m.toString().contains("msg2"));
 
-
         con.close();
         con2.close();
     }
 
-
     public void testReturnUnroutableMandatoryMessage_TOPIC() throws Exception
     {
         _bouncedMessageList.clear();
         Connection con = new AMQConnection(BROKER, "guest", "guest", "consumer1", VIRTUALHOST);
 
-
         AMQSession consumerSession = (AMQSession) con.createSession(false, Session.CLIENT_ACKNOWLEDGE);
 
         AMQTopic valid_topic = new AMQTopic(ExchangeDefaults.TOPIC_EXCHANGE_CLASS, "test.Return.Unroutable.Mandatory.Message.TOPIC");
@@ -240,7 +249,6 @@ public class ReturnUnroutableMandatoryMessageTest extends TestCase implements Ex
         _logger.info("Starting producer connection");
         con2.start();
 
-
         MessageProducer nonMandatoryProducer = producerSession.createProducer(valid_topic, false, false);
         MessageProducer mandatoryProducer = producerSession.createProducer(invalid_topic);
 
@@ -254,7 +262,6 @@ public class ReturnUnroutableMandatoryMessageTest extends TestCase implements Ex
         TextMessage msg2 = producerSession.createTextMessage("msg2");
         mandatoryProducer.send(msg2);
 
-
         _logger.info("Starting consumer connection");
         con.start();
         TextMessage tm = (TextMessage) consumer.receive(1000L);
@@ -275,12 +282,10 @@ public class ReturnUnroutableMandatoryMessageTest extends TestCase implements Ex
         Message m = _bouncedMessageList.get(0);
         assertTrue("Wrong message bounced: " + m.toString(), m.toString().contains("msg2"));
 
-
         con.close();
         con2.close();
     }
 
-
     public static junit.framework.Test suite()
     {
         return new junit.framework.TestSuite(ReturnUnroutableMandatoryMessageTest.class);
@@ -293,7 +298,8 @@ public class ReturnUnroutableMandatoryMessageTest extends TestCase implements Ex
         try
         {
             linkedException = jmsException.getLinkedException();
-        } catch (Exception e)
+        }
+        catch (Exception e)
         {
             e.printStackTrace();  //To change body of catch statement use File | Settings | File Templates.
         }
diff --git a/qpid/java/systests/src/main/java/org/apache/qpid/server/security/acl/SimpleACLTest.java b/qpid/java/systests/src/main/java/org/apache/qpid/server/security/acl/SimpleACLTest.java
index e39b1122cf..db29b2d5f9 100644
--- a/qpid/java/systests/src/main/java/org/apache/qpid/server/security/acl/SimpleACLTest.java
+++ b/qpid/java/systests/src/main/java/org/apache/qpid/server/security/acl/SimpleACLTest.java
@@ -35,7 +35,6 @@ import javax.jms.*;
 import javax.jms.IllegalStateException;
 import java.io.File;
 
-
 public class SimpleACLTest extends TestCase implements ConnectionListener
 {
     private String BROKER = "vm://:1";//"tcp://localhost:5672";
@@ -52,7 +51,7 @@ public class SimpleACLTest extends TestCase implements ConnectionListener
             fail("Configuration file not found:" + defaultaclConfigFile);
         }
 
-        if (System.getProperty("QPID_HOME") == null)
+        if (System.getProperty("QPID_HOME") == null)                                                                                            
         {
             fail("QPID_HOME not set");
         }
@@ -73,7 +72,7 @@ public class SimpleACLTest extends TestCase implements ConnectionListener
     public String createConnectionString(String username, String password, String broker)
     {
 
-        return "amqp://" + username + ":" + password + "@clientid/test?brokerlist='" + broker + "'";
+        return "amqp://" + username + ":" + password + "@clientid/test?brokerlist='" + broker + "?retries='0''";
     }
 
     public void testAccessAuthorized() throws AMQException, URLSyntaxException
@@ -113,15 +112,9 @@ public class SimpleACLTest extends TestCase implements ConnectionListener
         }
         catch (AMQException amqe)
         {
-            if (amqe.getCause().getClass() == Exception.class)
-            {
-                System.err.println("QPID-594 : WARNING RACE CONDITION. Unable to determine cause of Connection Failure.");
-                return;
-            }
-            assertEquals("Linked Exception Incorrect", JMSException.class, amqe.getCause().getClass());
-            Exception linked = ((JMSException) amqe.getCause()).getLinkedException();
-            assertEquals("Exception was wrong type", AMQAuthenticationException.class, linked.getClass());
-            assertEquals("Incorrect error code thrown", 403, ((AMQAuthenticationException) linked).getErrorCode().getCode());
+            Throwable cause = amqe.getCause();
+            assertEquals("Exception was wrong type", AMQAuthenticationException.class, cause.getClass());
+            assertEquals("Incorrect error code thrown", 403, ((AMQAuthenticationException) cause).getErrorCode().getCode());
         }
     }
 
@@ -304,11 +297,6 @@ public class SimpleACLTest extends TestCase implements ConnectionListener
 
             fail("Close is not expected to succeed.");
         }
-        catch (IllegalStateException ise)
-        {
-            System.err.println("QPID-826 : WARNING : Unable to determine cause of failure due to closure as we don't " +
-                               "record it for reporting after connection closed asynchronously");
-        }
         catch (JMSException e)
         {
             Throwable cause = e.getLinkedException();
@@ -567,15 +555,10 @@ public class SimpleACLTest extends TestCase implements ConnectionListener
 
             fail("Close is not expected to succeed.");
         }
-        catch (IllegalStateException ise)
-        {
-            System.err.println("QPID-826 : WARNING : Unable to determine cause of failure due to closure as we don't " +
-                               "record it for reporting after connection closed asynchronously");
-        }
         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());
         }
diff --git a/qpid/java/systests/src/main/java/org/apache/qpid/test/client/QueueBrowserAutoAckTest.java b/qpid/java/systests/src/main/java/org/apache/qpid/test/client/QueueBrowserAutoAckTest.java
index 67ebc6fde0..9c467c2198 100644
--- a/qpid/java/systests/src/main/java/org/apache/qpid/test/client/QueueBrowserAutoAckTest.java
+++ b/qpid/java/systests/src/main/java/org/apache/qpid/test/client/QueueBrowserAutoAckTest.java
@@ -159,6 +159,8 @@ public class QueueBrowserAutoAckTest extends FailoverBaseCase
 
         assertEquals("Session reports Queue depth not as expected", depth, queueDepth);
 
+
+
         // Browse the queue to get a second opinion
         int msgCount = 0;
         Enumeration msgs = queueBrowser.getEnumeration();
@@ -418,9 +420,14 @@ public class QueueBrowserAutoAckTest extends FailoverBaseCase
         validate(messages);
     }
 
+    /**
+     * Testing that a QueueBrowser doesn't actually consume messages from a broker when it fails over. 
+     * @throws JMSException
+     */
     public void testFailoverWithQueueBrowser() throws JMSException
     {
         int messages = 5;
+        
 
         sendMessages("connection1", messages);
         sendMessages("connection2", messages);
@@ -475,7 +482,9 @@ public class QueueBrowserAutoAckTest extends FailoverBaseCase
         //Close browser
         queueBrowser.close();
 
-        //Validate all messages still on Broker 1
+        _logger.info("Closed Queue Browser, validating messages on broker.");
+
+        //Validate all messages still on Broker
         validate(messages);
     }
 
@@ -495,21 +504,5 @@ public class QueueBrowserAutoAckTest extends FailoverBaseCase
         //Validate all messages still on Broker 1
         validate(messages);
     }
-
-    public void loop() throws JMSException
-    {
-        int run = 0;
-        try
-        {
-            while (true)
-            {
-                System.err.println(run++ + ":************************************************************************");
-                testQueueBrowserMsgsRemainOnQueue();
-            }
-        }
-        catch (Exception e)
-        {
-            _logger.error(e, e);
-        }
-    }
+        
 }
diff --git a/qpid/java/systests/src/main/java/org/apache/qpid/test/unit/client/connection/ConnectionTest.java b/qpid/java/systests/src/main/java/org/apache/qpid/test/unit/client/connection/ConnectionTest.java
index ed7a0f18b6..bb61966e3e 100644
--- a/qpid/java/systests/src/main/java/org/apache/qpid/test/unit/client/connection/ConnectionTest.java
+++ b/qpid/java/systests/src/main/java/org/apache/qpid/test/unit/client/connection/ConnectionTest.java
@@ -132,7 +132,7 @@ public class ConnectionTest extends QpidTestCase
         AMQConnection conn = null;
         try
         {
-            conn = new AMQConnection("amqp://guest:rubbishpassword@clientid/test?brokerlist='" + getBroker() + "?retries='1''");
+            conn = new AMQConnection("amqp://guest:rubbishpassword@clientid/test?brokerlist='" + getBroker() + "?retries='0''");
             fail("Connection should not be established password is wrong.");
         }
         catch (AMQConnectionFailureException amqe)