> 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
>
>
