-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/5794/#review9055
-----------------------------------------------------------
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.
- Oleksandr Rudyy
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
>
>
