QPID-1809, QPID-2081 : Corrected ChannelClose logic. Removed an unnecessary sync on the failoverMutex in AMQSession that was causing the notification of the close to be blocked until a TimeOutException occured.

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

2 files changed, 63 insertions, 57 deletions

diff --git a/java/client/src/main/java/org/apache/qpid/client/AMQSession.java b/java/client/src/main/java/org/apache/qpid/client/AMQSession.java
index dad656bd50..2e3e417c95 100644
--- a/java/client/src/main/java/org/apache/qpid/client/AMQSession.java
+++ b/java/client/src/main/java/org/apache/qpid/client/AMQSession.java
@@ -721,25 +721,22 @@ public abstract class AMQSession<C extends BasicMessageConsumer, P extends Basic
 
         if (!_closed.getAndSet(true))
         {
-            synchronized (getFailoverMutex())
+            synchronized (_messageDeliveryLock)
             {
-                synchronized (_messageDeliveryLock)
+                // An AMQException has an error code and message already and will be passed in when closure occurs as a
+                // result of a channel close request
+                AMQException amqe;
+                if (e instanceof AMQException)
                 {
-                    // An AMQException has an error code and message already and will be passed in when closure occurs as a
-                    // result of a channel close request
-                    AMQException amqe;
-                    if (e instanceof AMQException)
-                    {
-                        amqe = (AMQException) e;
-                    }
-                    else
-                    {
-                        amqe = new AMQException("Closing session forcibly", e);
-                    }
-
-                    _connection.deregisterSession(_channelId);
-                    closeProducersAndConsumers(amqe);
+                    amqe = (AMQException) e;
+                }
+                else
+                {
+                    amqe = new AMQException("Closing session forcibly", e);
                 }
+
+                _connection.deregisterSession(_channelId);
+                closeProducersAndConsumers(amqe);
             }
         }
     }
diff --git a/java/client/src/main/java/org/apache/qpid/client/handler/ChannelCloseMethodHandler.java b/java/client/src/main/java/org/apache/qpid/client/handler/ChannelCloseMethodHandler.java
index 2b6745ebe4..2cf19bf391 100644
--- a/java/client/src/main/java/org/apache/qpid/client/handler/ChannelCloseMethodHandler.java
+++ b/java/client/src/main/java/org/apache/qpid/client/handler/ChannelCloseMethodHandler.java
@@ -32,7 +32,6 @@ import org.apache.qpid.framing.AMQShortString;
 import org.apache.qpid.framing.ChannelCloseBody;
 import org.apache.qpid.framing.ChannelCloseOkBody;
 import org.apache.qpid.protocol.AMQConstant;
-
 import org.slf4j.Logger;
 import org.slf4j.LoggerFactory;
 
@@ -48,7 +47,7 @@ public class ChannelCloseMethodHandler implements StateAwareMethodListener<Chann
     }
 
     public void methodReceived(AMQProtocolSession session, ChannelCloseBody method, int channelId)
-        throws AMQException
+            throws AMQException
     {
         _logger.debug("ChannelClose method received");
 
@@ -59,52 +58,62 @@ public class ChannelCloseMethodHandler implements StateAwareMethodListener<Chann
             _logger.debug("Channel close reply code: " + errorCode + ", reason: " + reason);
         }
 
-
-
         ChannelCloseOkBody body = session.getMethodRegistry().createChannelCloseOkBody();
         AMQFrame frame = body.generateFrame(channelId);
         session.writeFrame(frame);
-
-        if (errorCode != AMQConstant.REPLY_SUCCESS)
+        try
         {
-            if (_logger.isDebugEnabled())
-            {
-                _logger.debug("Channel close received with errorCode " + errorCode + ", and reason " + reason);
-            }
-
-            if (errorCode == AMQConstant.NO_CONSUMERS)
-            {
-                throw new AMQNoConsumersException("Error: " + reason, null, null);
-            }
-            else if (errorCode == AMQConstant.NO_ROUTE)
-            {
-                throw new AMQNoRouteException("Error: " + reason, null, null);
-            }
-            else if (errorCode == AMQConstant.INVALID_ARGUMENT)
+            if (errorCode != AMQConstant.REPLY_SUCCESS)
             {
-                _logger.debug("Broker responded with Invalid Argument.");
+                if (_logger.isDebugEnabled())
+                {
+                    _logger.debug("Channel close received with errorCode " + errorCode + ", and reason " + reason);
+                }
+
+                if (errorCode == AMQConstant.NO_CONSUMERS)
+                {
+                    throw new AMQNoConsumersException("Error: " + reason, null, null);
+                }
+                else if (errorCode == AMQConstant.NO_ROUTE)
+                {
+                    throw new AMQNoRouteException("Error: " + reason, null, null);
+                }
+                else if (errorCode == AMQConstant.INVALID_ARGUMENT)
+                {
+                    _logger.debug("Broker responded with Invalid Argument.");
+
+                    throw new org.apache.qpid.AMQInvalidArgumentException(String.valueOf(reason), null);
+                }
+                else if (errorCode == AMQConstant.INVALID_ROUTING_KEY)
+                {
+                    _logger.debug("Broker responded with Invalid Routing Key.");
+
+                    throw new AMQInvalidRoutingKeyException(String.valueOf(reason), null);
+                }
+                else
+                {
+
+                    throw new AMQChannelClosedException(errorCode, "Error: " + reason, null);
+                }
 
-                throw new org.apache.qpid.AMQInvalidArgumentException(String.valueOf(reason), null);
-            }
-            else if (errorCode == AMQConstant.INVALID_ROUTING_KEY)
-            {
-                _logger.debug("Broker responded with Invalid Routing Key.");
-
-                throw new AMQInvalidRoutingKeyException(String.valueOf(reason), null);
-            }
-            else
-            {
-                throw new AMQChannelClosedException(errorCode, "Error: " + reason, null);
             }
-
         }
-        // fixme why is this only done when the close is expected...
-        // should the above forced closes not also cause a close?
-        // ----------
-        // Closing the session only when it is expected allows the errors to be processed
-        // Calling this here will prevent failover. So we should do this for all exceptions
-        // that should never cause failover. Such as authentication errors.
-
-        session.channelClosed(channelId, errorCode, String.valueOf(reason));
+        finally
+        {
+            // fixme why is this only done when the close is expected...
+            // should the above forced closes not also cause a close?
+            // ----------
+            // Closing the session only when it is expected allows the errors to be processed
+            // Calling this here will prevent failover. So we should do this for all exceptions
+            // that should never cause failover. Such as authentication errors.
+            // ----
+            // 2009-09-07 - ritchiem
+            // calling channelClosed will only close this session and will not
+            // prevent failover. If we don't close the session here then we will
+            // have problems during the session close as it will attempt to
+            // close the session that the broker has closed, 
+
+            session.channelClosed(channelId, errorCode, String.valueOf(reason));
+        }
     }
 }