qpid-dev mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From "rajith attapattu" <rajit...@gmail.com>
Subject Re: Review Request: Fix for QPID-3575
Date Wed, 11 Jul 2012 15:20:43 GMT


> On July 11, 2012, 11:42 a.m., Oleksandr Rudyy wrote:
> > Hi Rajith,
> > 
> > The suggested code changes looks reasonable for me.
> > I could not find any issue with the suggested solution.
> > 
> > The only thing I would like to ask is to add some tests to cover the changes.
> > For example, I used the following tests to test your changes with Java Broker
> > 
> > public class ExceptionHandlingTest extends QpidBrokerTestCase implements ExceptionListener
> > {
> > 
> >     private AMQConnection _connection;
> >     private AMQSession_0_10 _exceptionSession;
> >     private AMQSession_0_10 _session;
> >     private JMSException _exception;
> > 
> >     public void setUp() throws Exception
> >     {
> >         super.setUp();
> >         _connection = (AMQConnection) getConnection();
> >         _connection.setExceptionListener(this);
> >         _session = (AMQSession_0_10) _connection.createSession(false, Session.AUTO_ACKNOWLEDGE);
> >         _exceptionSession = (AMQSession_0_10) _connection.createSession(false, Session.AUTO_ACKNOWLEDGE);
> >         _exception = null;
> >     }
> > 
> >     public void testDeclareStandardEchange() throws Exception
> >     {
> >         Exception caughtException = null;
> >         try
> >         {
> >             _exceptionSession.declareExchange(new AMQShortString("qpid.management"),
new AMQShortString("direct"), false);
> >         }
> >         catch (Exception e)
> >         {
> >             e.printStackTrace();
> >             caughtException = e;
> >         }
> >         assertState();
> >         assertTrue("Unexpected exception", caughtException == null || caughtException
instanceof AMQException);
> >     }
> > 
> >     public void testDeclareQueuePassiveForNonExistingQueue() throws Exception
> >     {
> >         Exception caughtException = null;
> >         try
> >         {
> >             _exceptionSession.send0_10QueueDeclare((AMQDestination) getTestQueue(),
_connection.getProtocolHandler(), false,
> >                     false, true);
> >         }
> >         catch (Exception e)
> >         {
> >             e.printStackTrace();
> >             caughtException = e;
> >         }
> >         assertState();
> >         assertTrue("Unexpected exception", caughtException == null || caughtException
instanceof AMQException);
> >     }
> > 
> >     @Override
> >     public void onException(JMSException exception)
> >     {
> >         _exception = exception;
> >     }
> > 
> >     private void assertState() throws Exception
> >     {
> >         assertFalse("Connection has been closed unexpectedly", _connection.isClosed());
> >         assertFalse("Not affected session has been closed unexpectedly", _session.isClosed());
> >         assertNotNull("Exception is not received", _exception);
> >         assertTrue("Exception session has not been closed", _exceptionSession.isClosed());
> > 
> >         // assert that JMS operations are performed OK on non-affected session
> >         Destination queue = _session.createQueue(getTestQueueName());
> >         MessageConsumer consumer = _session.createConsumer(queue);
> >         MessageProducer producer = _session.createProducer(queue);
> >         producer.send(_session.createTextMessage("Test"));
> >         _connection.start();
> >         Message m = consumer.receive(1000l);
> >         assertNotNull("Test message has not been received", m);
> >         consumer.close();
> >         producer.close();
> >     }
> > }
> > 
> > I did not try to run these tests with CPP broker.
> > 
> > The failing test SelectorTest#testRuntimeSelectorError can be fixed by replacing
the assert checking if connection is closed 
> > assertTrue("Connection should be closed", _connection.isClosed());
> > with assert checking if session is closed
> > assertTrue("Session should be closed", (AMQSession<?,?)session.isClosed());
> > 
> > 
> > Apart from adding the tests, I do not have any other commentaries about the suggested
changes.

Thanks Alex!
I will add the tests before I commit.


- rajith


-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/5794/#review9055
-----------------------------------------------------------


On July 6, 2012, 12:39 a.m., rajith attapattu wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/5794/
> -----------------------------------------------------------
> 
> (Updated July 6, 2012, 12:39 a.m.)
> 
> 
> Review request for qpid, Robbie Gemmell, Keith Wall, and Rob Godfrey.
> 
> 
> Description
> -------
> 
> When a session exception is received, we notify the connection via the exceptionReceived
method. The connection will close all sessions for this connection, but does not close the
underlying Protocol connection.
> Further calls (from application code) to close the connection, does not close the underlying
protocol-connection as it skips this part, due to the connection already being marked as closed.
> This results in leaked connections.
> 
> The proposed patch does the following.
> 1. When a session exception is received, it closes the Session (and the consumers/producers
associated with it) to prevent further use of this session.
> 2. It also notifies the connection, however the exception being passed, is marked as
a soft error, thereby preventing the connection from being marked as closed.
>    It also prevents the rest of the sessions being closed.
> 3. The exception will however be notified via the exception listener. This is important
in certain cases, as it might be the only case where an exception can be notified.
> 
> What is the impact to applications.
> ------------------------------------
> 1. If an application closes the connection when it receives an exception, and then recreates
the connection and it's associated objects (sessions/consumers/producers) then this change
will have no impact.
> 
> 2. If an application just recreates everything from scratch, without resorting to closing
the connection (assuming that all exceptions received via the listener will automatically
close the connection), there will be tcp connection leaks. 
> However the objects associated with the connection will eventualy be garbage collected.
We should probably close the underlying TCP connection in the finalizer for the Connection
object as a safety measure.
> 
> With this change, the recommended approach would be, for application to check the connection
status in deciding what it should do.
> For example, 
> 
> if (conn.isClosed())
> {
>    // re-create connection.
> }
> else
> {
>   // session exception
>   // this means the rest of the sessions and the connection is still open.
> }
> 
> 
> This addresses bug QPID-3575.
>     https://issues.apache.org/jira/browse/QPID-3575
> 
> 
> Diffs
> -----
> 
>   http://svn.apache.org/repos/asf/qpid/trunk/qpid/java/client/src/main/java/org/apache/qpid/client/AMQSession.java
1357981 
>   http://svn.apache.org/repos/asf/qpid/trunk/qpid/java/client/src/main/java/org/apache/qpid/client/AMQSession_0_10.java
1357981 
>   http://svn.apache.org/repos/asf/qpid/trunk/qpid/java/common/src/main/java/org/apache/qpid/AMQException.java
1357981 
> 
> Diff: https://reviews.apache.org/r/5794/diff/
> 
> 
> Testing
> -------
> 
> Tested with the default java profile and the cpp test profile.
> 
> SelectorTest#testRuntimeSelectorError is failing as it expects the connection to be closed.
> I need to investigate further to determine if the test needs to be changed or not.
> 
> 
> Thanks,
> 
> rajith attapattu
> 
>


Mime
  • Unnamed multipart/alternative (inline, None, 0 bytes)
View raw message