From e5521a71e0171567e29395b5ba555004635beae1 Mon Sep 17 00:00:00 2001 From: Martin Ritchie Date: Fri, 4 Sep 2009 16:27:54 +0000 Subject: QPID-1809 - The incorrect expcetions were due to a race condition between the mina exception notification thread and the clients main thread blocking for a frame. Occasionally, the client will start blocking just after the notification and so will Timeout. This update ensures that blocking does not occur if the connection has been marked closing or is closed. The lastException set on the StateManager is thrown instead. The connection close also needed to take into consideration this fact. The syncWrite on for ChannelClose and ConnectionClose are now only down if we are not in a closing situation. As the 0-10 code path does not use the StateManager the changes were applied to the 0-8 Session. Further testing may be needed to validate that the 0-10 client code path does not also have this race condition. git-svn-id: https://svn.apache.org/repos/asf/qpid/trunk@811471 13f79535-47bb-0310-9956-ffa450edef68 --- .../qpid/client/AMQAuthenticationException.java | 5 -- .../org/apache/qpid/client/AMQSession_0_8.java | 24 ++++++-- .../handler/ConnectionOpenOkMethodHandler.java | 1 - .../handler/ConnectionTuneMethodHandler.java | 1 - .../protocol/AMQIoTransportProtocolSession.java | 2 +- .../qpid/client/protocol/AMQProtocolHandler.java | 54 +++++++---------- .../qpid/client/protocol/AMQProtocolSession.java | 4 +- .../apache/qpid/client/state/AMQStateManager.java | 19 +++++- .../org/apache/qpid/client/MockAMQConnection.java | 2 +- .../client/protocol/AMQProtocolHandlerTest.java | 11 +--- .../close/JavaServerCloseRaceConditionTest.java | 69 ++++++++++++++++++++++ 11 files changed, 133 insertions(+), 59 deletions(-) create mode 100644 qpid/java/systests/src/main/java/org/apache/qpid/test/unit/close/JavaServerCloseRaceConditionTest.java (limited to 'qpid/java') diff --git a/qpid/java/client/src/main/java/org/apache/qpid/client/AMQAuthenticationException.java b/qpid/java/client/src/main/java/org/apache/qpid/client/AMQAuthenticationException.java index 05ac3dca9e..6bae0166d1 100644 --- a/qpid/java/client/src/main/java/org/apache/qpid/client/AMQAuthenticationException.java +++ b/qpid/java/client/src/main/java/org/apache/qpid/client/AMQAuthenticationException.java @@ -39,9 +39,4 @@ public class AMQAuthenticationException extends AMQException { super(error, msg, cause); } - public boolean isHardError() - { - return true; - } - } diff --git a/qpid/java/client/src/main/java/org/apache/qpid/client/AMQSession_0_8.java b/qpid/java/client/src/main/java/org/apache/qpid/client/AMQSession_0_8.java index fa4e08f62b..d7196c0abb 100644 --- a/qpid/java/client/src/main/java/org/apache/qpid/client/AMQSession_0_8.java +++ b/qpid/java/client/src/main/java/org/apache/qpid/client/AMQSession_0_8.java @@ -33,6 +33,7 @@ import org.apache.qpid.client.message.*; import org.apache.qpid.client.message.AMQMessageDelegateFactory; import org.apache.qpid.client.protocol.AMQProtocolHandler; import org.apache.qpid.client.state.listener.SpecificMethodFrameListener; +import org.apache.qpid.client.state.AMQState; import org.apache.qpid.common.AMQPFilterTypes; import org.apache.qpid.framing.*; import org.apache.qpid.framing.amqp_0_9.MethodRegistry_0_9; @@ -116,12 +117,23 @@ public final class AMQSession_0_8 extends AMQSession @@ -86,7 +87,7 @@ public class AMQStateManager implements AMQMethodListener return _currentState; } - public void changeState(AMQState newState) throws AMQException + public void changeState(AMQState newState) { _logger.debug("State changing to " + newState + " from old state " + _currentState); @@ -136,6 +137,22 @@ public class AMQStateManager implements AMQMethodListener */ public void error(Exception error) { + if (error instanceof AMQException) + { + // AMQException should be being notified before closing the + // ProtocolSession. Which will change the State to CLOSED. + // if we have a hard error. + if (((AMQException)error).isHardError()) + { + changeState(AMQState.CONNECTION_CLOSING); + } + } + else + { + // Be on the safe side here and mark the connection closed + changeState(AMQState.CONNECTION_CLOSED); + } + if (_waiters.size() == 0) { _logger.error("No Waiters for error saving as last error:" + error.getMessage()); diff --git a/qpid/java/client/src/test/java/org/apache/qpid/client/MockAMQConnection.java b/qpid/java/client/src/test/java/org/apache/qpid/client/MockAMQConnection.java index ce79080e97..da44822ec3 100644 --- a/qpid/java/client/src/test/java/org/apache/qpid/client/MockAMQConnection.java +++ b/qpid/java/client/src/test/java/org/apache/qpid/client/MockAMQConnection.java @@ -85,7 +85,7 @@ public class MockAMQConnection extends AMQConnection } @Override - public ProtocolVersion makeBrokerConnection(BrokerDetails brokerDetail) throws IOException, AMQException + public ProtocolVersion makeBrokerConnection(BrokerDetails brokerDetail) throws IOException { _connected = true; _protocolHandler.getStateManager().changeState(AMQState.CONNECTION_OPEN); diff --git a/qpid/java/client/src/test/java/org/apache/qpid/client/protocol/AMQProtocolHandlerTest.java b/qpid/java/client/src/test/java/org/apache/qpid/client/protocol/AMQProtocolHandlerTest.java index 10ec220d9e..fc7f8148f0 100644 --- a/qpid/java/client/src/test/java/org/apache/qpid/client/protocol/AMQProtocolHandlerTest.java +++ b/qpid/java/client/src/test/java/org/apache/qpid/client/protocol/AMQProtocolHandlerTest.java @@ -200,15 +200,8 @@ public class AMQProtocolHandlerTest extends TestCase _handler.getStateManager().error(trigger); _logger.info("Setting state to be CONNECTION_CLOSED."); - try - { - _handler.getStateManager().changeState(AMQState.CONNECTION_CLOSED); - } - catch (AMQException e) - { - _logger.error("Unable to change the state to closed.", e); - fail("Unable to change the state to closed due to :"+e.getMessage()); - } + + _handler.getStateManager().changeState(AMQState.CONNECTION_CLOSED); _logger.info("Firing exception"); _handler.propagateExceptionToFrameListeners(trigger); diff --git a/qpid/java/systests/src/main/java/org/apache/qpid/test/unit/close/JavaServerCloseRaceConditionTest.java b/qpid/java/systests/src/main/java/org/apache/qpid/test/unit/close/JavaServerCloseRaceConditionTest.java new file mode 100644 index 0000000000..7ff603df96 --- /dev/null +++ b/qpid/java/systests/src/main/java/org/apache/qpid/test/unit/close/JavaServerCloseRaceConditionTest.java @@ -0,0 +1,69 @@ +/* + * + * Licensed to the Apache Software Foundation (ASF) under one + * or more contributor license agreements. See the NOTICE file + * distributed with this work for additional information + * regarding copyright ownership. The ASF licenses this file + * to you under the Apache License, Version 2.0 (the + * "License"); you may not use this file except in compliance + * with the License. You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, + * software distributed under the License is distributed on an + * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY + * KIND, either express or implied. See the License for the + * specific language governing permissions and limitations + * under the License. + * + */ +package org.apache.qpid.test.unit.close; + +import org.apache.qpid.client.AMQAuthenticationException; +import org.apache.qpid.client.AMQConnection; +import org.apache.qpid.client.AMQDestination; +import org.apache.qpid.client.AMQSession; +import org.apache.qpid.framing.AMQFrame; +import org.apache.qpid.framing.AMQShortString; +import org.apache.qpid.framing.ExchangeDeclareBody; +import org.apache.qpid.framing.ExchangeDeclareOkBody; +import org.apache.qpid.test.utils.QpidTestCase; + +import javax.jms.Session; + +/** QPID-1085 */ +public class JavaServerCloseRaceConditionTest extends QpidTestCase +{ + public void test() throws Exception + { + + AMQConnection connection = (AMQConnection) getConnection(); + + AMQSession session = (AMQSession) connection.createSession(false, Session.AUTO_ACKNOWLEDGE); + + AMQDestination destination = (AMQDestination) session.createQueue(getTestQueueName()); + + // Set no wait true so that we block the connection + // Also set a different exchange class string so the attempt to declare + // the exchange causes an exchange. + ExchangeDeclareBody body = session.getMethodRegistry().createExchangeDeclareBody(session.getTicket(), destination.getExchangeName(), new AMQShortString("NewTypeForException"), + destination.getExchangeName().toString().startsWith("amq."), + false, false, false, true, null); + + AMQFrame exchangeDeclare = body.generateFrame(session.getChannelId()); + + try + { + // block our thread so that can times out + connection.getProtocolHandler().syncWrite(exchangeDeclare, ExchangeDeclareOkBody.class); + } + catch (Exception e) + { + if (!(e instanceof AMQAuthenticationException)) + { + fail("Cause was not AMQAuthenticationException. Was " + e.getClass() + ":" + e.getMessage()); + } + } + } +} -- cgit v1.2.1