From qpid-dev-return-10676-apmail-incubator-qpid-dev-archive=incubator.apache.org@incubator.apache.org Wed Jul 02 10:52:01 2008 Return-Path: Delivered-To: apmail-incubator-qpid-dev-archive@locus.apache.org Received: (qmail 7325 invoked from network); 2 Jul 2008 10:52:00 -0000 Received: from hermes.apache.org (HELO mail.apache.org) (140.211.11.2) by minotaur.apache.org with SMTP; 2 Jul 2008 10:52:00 -0000 Received: (qmail 53147 invoked by uid 500); 2 Jul 2008 10:52:02 -0000 Delivered-To: apmail-incubator-qpid-dev-archive@incubator.apache.org Received: (qmail 53023 invoked by uid 500); 2 Jul 2008 10:52:01 -0000 Mailing-List: contact qpid-dev-help@incubator.apache.org; run by ezmlm Precedence: bulk List-Help: List-Unsubscribe: List-Post: List-Id: Reply-To: qpid-dev@incubator.apache.org Delivered-To: mailing list qpid-dev@incubator.apache.org Received: (qmail 53012 invoked by uid 99); 2 Jul 2008 10:52:01 -0000 Received: from athena.apache.org (HELO athena.apache.org) (140.211.11.136) by apache.org (qpsmtpd/0.29) with ESMTP; Wed, 02 Jul 2008 03:52:01 -0700 X-ASF-Spam-Status: No, hits=-4.0 required=10.0 tests=RCVD_IN_DNSWL_MED,SPF_HELO_PASS,SPF_PASS X-Spam-Check-By: apache.org Received-SPF: pass (athena.apache.org: domain of rafaels@redhat.com designates 66.187.233.31 as permitted sender) Received: from [66.187.233.31] (HELO mx1.redhat.com) (66.187.233.31) by apache.org (qpsmtpd/0.29) with ESMTP; Wed, 02 Jul 2008 10:51:11 +0000 Received: from int-mx1.corp.redhat.com (int-mx1.corp.redhat.com [172.16.52.254]) by mx1.redhat.com (8.13.8/8.13.8) with ESMTP id m62AnTT9006575 for ; Wed, 2 Jul 2008 06:49:29 -0400 Received: from mail.boston.redhat.com (mail.boston.redhat.com [10.16.255.12]) by int-mx1.corp.redhat.com (8.13.1/8.13.1) with ESMTP id m62AnT1Z031389 for ; Wed, 2 Jul 2008 06:49:29 -0400 Received: from planitia.bos.redhat.com (planitia.bos.redhat.com [10.16.16.54]) by mail.boston.redhat.com (8.13.1/8.13.1) with ESMTP id m62AnSZR019257 for ; Wed, 2 Jul 2008 06:49:29 -0400 Message-ID: <486B5CDE.3060307@redhat.com> Date: Wed, 02 Jul 2008 06:47:58 -0400 From: Rafael Schloming User-Agent: Thunderbird 2.0.0.9 (X11/20071115) MIME-Version: 1.0 To: qpid-dev@incubator.apache.org Subject: Re: svn commit: r673343 - in /incubator/qpid/trunk/qpid/java/client/src: main/java/org/apache/qpid/client/ main/java/org/apache/qpid/client/protocol/ main/java/org/apache/qpid/client/state/ test/java/org/apache/qpid/test/unit/client/connection/ References: <20080702100550.DE2482388A16@eris.apache.org> In-Reply-To: <20080702100550.DE2482388A16@eris.apache.org> Content-Type: text/plain; charset=UTF-8; format=flowed Content-Transfer-Encoding: 7bit X-Scanned-By: MIMEDefang 2.58 on 172.16.52.254 X-Virus-Checked: Checked by ClamAV on apache.org I don't recall the details, but I'm pretty sure there are some circumstances when multiple exceptions get passed through this listener. I think we should do something to ensure that exceptions aren't lost under those circumstances, as it's *really* confusing when they are. --Rafael aidan@apache.org wrote: > Author: aidan > Date: Wed Jul 2 03:05:49 2008 > New Revision: 673343 > > URL: http://svn.apache.org/viewvc?rev=673343&view=rev > Log: > QPID-962 Exception handling was... unpleasing... Fix up of patch from rhs > > AMQConnection: Refactor listener and remove list, we're only interested in the most recent one anyway. Add get/set for lastException, which can now be any Exception > > AMQConnectionDelegate_0_8.java: Stop masking/stackign exceptions, just throw them. > > AMQProtocolHandler.java: attainState can now throw any sort of Exception > > AMQStateManager.java: attainState can now throw any Exception > > ConnectionTest.java: check that exception cause is not null > > Modified: > incubator/qpid/trunk/qpid/java/client/src/main/java/org/apache/qpid/client/AMQConnection.java > incubator/qpid/trunk/qpid/java/client/src/main/java/org/apache/qpid/client/AMQConnectionDelegate_0_8.java > incubator/qpid/trunk/qpid/java/client/src/main/java/org/apache/qpid/client/protocol/AMQProtocolHandler.java > incubator/qpid/trunk/qpid/java/client/src/main/java/org/apache/qpid/client/state/AMQStateManager.java > incubator/qpid/trunk/qpid/java/client/src/test/java/org/apache/qpid/test/unit/client/connection/ConnectionTest.java > > Modified: incubator/qpid/trunk/qpid/java/client/src/main/java/org/apache/qpid/client/AMQConnection.java > URL: http://svn.apache.org/viewvc/incubator/qpid/trunk/qpid/java/client/src/main/java/org/apache/qpid/client/AMQConnection.java?rev=673343&r1=673342&r2=673343&view=diff > ============================================================================== > --- incubator/qpid/trunk/qpid/java/client/src/main/java/org/apache/qpid/client/AMQConnection.java (original) > +++ incubator/qpid/trunk/qpid/java/client/src/main/java/org/apache/qpid/client/AMQConnection.java Wed Jul 2 03:05:49 2008 > @@ -26,6 +26,7 @@ > import org.apache.qpid.AMQUnresolvedAddressException; > import org.apache.qpid.client.failover.FailoverException; > import org.apache.qpid.client.protocol.AMQProtocolHandler; > +import org.apache.qpid.client.state.AMQState; > import org.apache.qpid.client.configuration.ClientProperties; > import org.apache.qpid.exchange.ExchangeDefaults; > import org.apache.qpid.framing.*; > @@ -234,7 +235,7 @@ > /* > * The last error code that occured on the connection. Used to return the correct exception to the client > */ > - protected AMQException _lastAMQException = null; > + protected Exception _lastException = null; > > /* > * The connection meta data > @@ -378,13 +379,20 @@ > _delegate = new AMQConnectionDelegate_0_10(this); > } > > - final ArrayList exceptions = new ArrayList(); > - > class Listener implements ExceptionListener > { > public void onException(JMSException e) > { > - exceptions.add(e); > + _lastException = e; > + try > + { > + getProtocolHandler().getStateManager().changeState(AMQState.CONNECTION_CLOSED); > + > + } > + catch (AMQException e1) > + { > + // Wow, badness > + } > } > } > > @@ -443,9 +451,6 @@ > // We are not currently connected > _connected = false; > > - Exception lastException = new Exception(); > - lastException.initCause(new ConnectException()); > - > // TMG FIXME this seems... wrong... > boolean retryAllowed = true; > while (!_connected && retryAllowed ) > @@ -453,8 +458,6 @@ > try > { > makeBrokerConnection(brokerDetails); > - lastException = null; > - _connected = true; > } > catch (AMQProtocolException pe) > { > @@ -470,12 +473,14 @@ > } > catch (Exception e) > { > - lastException = e; > - > + _lastException = e; > + } > + if (_lastException != null) > + { > if (_logger.isInfoEnabled()) > { > _logger.info("Unable to connect to broker at " + _failoverPolicy.getCurrentBrokerDetails(), > - e.getCause()); > + _lastException.getCause()); > } > retryAllowed = _failoverPolicy.failoverAllowed(); > brokerDetails = _failoverPolicy.getNextBrokerDetails(); > @@ -498,31 +503,16 @@ > { > // Eat it, we've hopefully got all the exceptions if this happened > } > - if (exceptions.size() > 0) > - { > - JMSException e = exceptions.get(0); > - int code = -1; > - try > - { > - code = new Integer(e.getErrorCode()).intValue(); > - } > - catch (NumberFormatException nfe) > - { > - // Ignore this, we have some error codes and messages swapped around > - } > - > - throw new AMQConnectionFailureException(AMQConstant.getConstant(code), > - e.getMessage(), e); > - } > - else if (lastException != null) > + > + if (_lastException != null) > { > - if (lastException.getCause() != null) > + if (_lastException.getCause() != null) > { > - message = lastException.getCause().getMessage(); > + message = _lastException.getCause().getMessage(); > } > else > { > - message = lastException.getMessage(); > + message = _lastException.getMessage(); > } > } > > @@ -534,24 +524,19 @@ > } > else // can only be "" if getMessage() returned it therfore lastException != null > { > - message = "Unable to Connect:" + lastException.getClass(); > + message = "Unable to Connect:" + _lastException.getClass(); > } > } > > - AMQException e = new AMQConnectionFailureException(message, null); > + AMQException e = new AMQConnectionFailureException(message, _lastException); > > - if (lastException != null) > + if (_lastException != null) > { > - if (lastException instanceof UnresolvedAddressException) > + if (_lastException instanceof UnresolvedAddressException) > { > e = new AMQUnresolvedAddressException(message, _failoverPolicy.getCurrentBrokerDetails().toString(), > null); > } > - > - if (e.getCause() != null) > - { > - e.initCause(lastException); > - } > } > > throw e; > @@ -1507,4 +1492,14 @@ > { > return _syncPersistence; > } > + > + public Exception getLastException() > + { > + return _lastException; > + } > + > + public void setLastException(Exception exception) > + { > + _lastException = exception; > + } > } > > Modified: incubator/qpid/trunk/qpid/java/client/src/main/java/org/apache/qpid/client/AMQConnectionDelegate_0_8.java > URL: http://svn.apache.org/viewvc/incubator/qpid/trunk/qpid/java/client/src/main/java/org/apache/qpid/client/AMQConnectionDelegate_0_8.java?rev=673343&r1=673342&r2=673343&view=diff > ============================================================================== > --- incubator/qpid/trunk/qpid/java/client/src/main/java/org/apache/qpid/client/AMQConnectionDelegate_0_8.java (original) > +++ incubator/qpid/trunk/qpid/java/client/src/main/java/org/apache/qpid/client/AMQConnectionDelegate_0_8.java Wed Jul 2 03:05:49 2008 > @@ -25,7 +25,9 @@ > import java.nio.channels.UnresolvedAddressException; > import java.text.MessageFormat; > import java.util.ArrayList; > +import java.util.EnumSet; > import java.util.Iterator; > +import java.util.Set; > > import javax.jms.JMSException; > import javax.jms.XASession; > @@ -76,24 +78,23 @@ > return ((cause instanceof ConnectException) || (cause instanceof UnresolvedAddressException)); > } > > - public void makeBrokerConnection(BrokerDetails brokerDetail) throws IOException, AMQException > + public void makeBrokerConnection(BrokerDetails brokerDetail) throws AMQException, IOException > { > - try > + final Set openOrClosedStates = > + EnumSet.of(AMQState.CONNECTION_OPEN, AMQState.CONNECTION_CLOSED); > + > + TransportConnection.getInstance(brokerDetail).connect(_conn._protocolHandler, brokerDetail); > + // this blocks until the connection has been set up or when an error > + // has prevented the connection being set up > + > + AMQState state = _conn._protocolHandler.attainState(openOrClosedStates); > + if(state == AMQState.CONNECTION_OPEN) > { > - TransportConnection.getInstance(brokerDetail).connect(_conn._protocolHandler, brokerDetail); > - // this blocks until the connection has been set up or when an error > - // has prevented the connection being set up > - _conn._protocolHandler.attainState(AMQState.CONNECTION_OPEN); > _conn._failoverPolicy.attainedConnection(); > > // Again this should be changed to a suitable notify > _conn._connected = true; > - } > - catch (AMQException e) > - { > - _conn._lastAMQException = e; > - throw e; > - } > + } > } > > public org.apache.qpid.jms.Session createSession(final boolean transacted, final int acknowledgeMode, final int prefetch) > > Modified: incubator/qpid/trunk/qpid/java/client/src/main/java/org/apache/qpid/client/protocol/AMQProtocolHandler.java > URL: http://svn.apache.org/viewvc/incubator/qpid/trunk/qpid/java/client/src/main/java/org/apache/qpid/client/protocol/AMQProtocolHandler.java?rev=673343&r1=673342&r2=673343&view=diff > ============================================================================== > --- incubator/qpid/trunk/qpid/java/client/src/main/java/org/apache/qpid/client/protocol/AMQProtocolHandler.java (original) > +++ incubator/qpid/trunk/qpid/java/client/src/main/java/org/apache/qpid/client/protocol/AMQProtocolHandler.java Wed Jul 2 03:05:49 2008 > @@ -559,7 +559,7 @@ > _frameListeners.remove(listener); > } > */ > - public void attainState(AMQState s) throws AMQException > + public void attainState(AMQState s) throws Exception > { > getStateManager().attainState(s); > } > > Modified: incubator/qpid/trunk/qpid/java/client/src/main/java/org/apache/qpid/client/state/AMQStateManager.java > URL: http://svn.apache.org/viewvc/incubator/qpid/trunk/qpid/java/client/src/main/java/org/apache/qpid/client/state/AMQStateManager.java?rev=673343&r1=673342&r2=673343&view=diff > ============================================================================== > --- incubator/qpid/trunk/qpid/java/client/src/main/java/org/apache/qpid/client/state/AMQStateManager.java (original) > +++ incubator/qpid/trunk/qpid/java/client/src/main/java/org/apache/qpid/client/state/AMQStateManager.java Wed Jul 2 03:05:49 2008 > @@ -102,7 +102,7 @@ > } > > > - public void attainState(final AMQState s) throws AMQException > + public void attainState(final AMQState s) throws Exception > { > synchronized (_stateLock) > { > @@ -118,6 +118,11 @@ > catch (InterruptedException e) > { > _logger.warn("Thread interrupted"); > + if (_protocolSession.getAMQConnection().getLastException() != null) > + { > + throw _protocolSession.getAMQConnection().getLastException(); > + } > + > } > > if (_currentState != s) > @@ -169,6 +174,11 @@ > catch (InterruptedException e) > { > _logger.warn("Thread interrupted"); > + if (_protocolSession.getAMQConnection().getLastException() != null) > + { > + throw new AMQException(null, "Could not attain state due to exception", > + _protocolSession.getAMQConnection().getLastException()); > + } > } > > if (!stateSet.contains(_currentState)) > > Modified: incubator/qpid/trunk/qpid/java/client/src/test/java/org/apache/qpid/test/unit/client/connection/ConnectionTest.java > URL: http://svn.apache.org/viewvc/incubator/qpid/trunk/qpid/java/client/src/test/java/org/apache/qpid/test/unit/client/connection/ConnectionTest.java?rev=673343&r1=673342&r2=673343&view=diff > ============================================================================== > --- incubator/qpid/trunk/qpid/java/client/src/test/java/org/apache/qpid/test/unit/client/connection/ConnectionTest.java (original) > +++ incubator/qpid/trunk/qpid/java/client/src/test/java/org/apache/qpid/test/unit/client/connection/ConnectionTest.java Wed Jul 2 03:05:49 2008 > @@ -134,6 +134,7 @@ > } > catch (AMQException amqe) > { > + assertNotNull("No cause set", amqe.getCause()); > if (amqe.getCause().getClass() == Exception.class) > { > System.err.println("QPID-594 : WARNING RACE CONDITION. Unable to determine cause of Connection Failure."); > >