Re: Review Request: QPID-3214 - suggested fix for the deadlock issue.

[rajith attapattu <ra...@gmail.com>]

> On 2011-04-21 08:39:41, Gordon Sim wrote:
> > That's a suggested fix for QPID-3214 right (not QPID-3216)?
> > 
> > I'm not that familiar with the code, but based on point 1 in your comment on QPID-3214 I have a few questions:
> > 
> > Not signalling session exceptions to the listener would be a semantic change, right? Might some applications be relying on that at present? Might there be cases where that was actually the only way to get informed of a session issue? E.g. if a session just has a message listener set and if the/a queue subscribed to is then deleted, a not-found exception will be sent by the broker. Will the application detect that?
> 
> rajith attapattu wrote:
>     Sorry it was a typo - yes it should be QPID-3214.
>     I have modified the title and the description.
>     
>     As mentioned in the JIRA, the exception handling is wrong!
>     It clearly violates both the JMS spec as well as the AMQP spec.
>     
>     1. The JMS API clearly states that only "serious problem with a Connection object" should be notified via the exception listener.
>        A session closed (for ex. due to resource exhaustion or ACL violation) should not close the rest of the sessions along with the connection.
>     
>     2. The AMQP spec states that execution exceptions are soft errors and should not result in the connection (and the rest of the sessions) being closed.
>     
>     While I agree that my suggested patch would be a semantic change, it is indeed the correct behaviour.
>     
>     I did some testing to see if we report session exceptions properly (in the 0-10 code path and this patch only affects 0-10).
>     It seems we do throw a JMS exception on a create producer or consumer if there is an ACL violation.
>     All actions in JMS (exception for message producing unless explicitly marked synchronous) is synchronous and we do throw a JMS exception if something goes wrong.
>     
>     We do throw a JMS exception on send method if the queue runs out of space, however due to the async nature it will be reported on a subsequent send.
>     However if we send just one more message over the limit the session does get closed, but an exception is not thrown for some subsequent operations on the closed session. Ex creating a producer. This is a bug as there is no check to see if the session is alive when creating a producer.
>     
>     If we can fix those minor issues, then I strongly suggest that my patch is the way to go as it restore the correct behaviour.
> 
> Gordon Sim wrote:
>     What about the case above, i.e. you have a session with message listener attached that is waiting for messages from a queue and that queue is then deleted. Does the session get notified or does it hang around forever? May be a fringe case we don't care about of course, but would be nice to be explicit about it.
> 
> Gordon Sim wrote:
>     > 1. The JMS API clearly states that only "serious problem with a Connection object" should be notified via the exception listener.
>     >   A session closed (for ex. due to resource exhaustion or ACL violation) should not close the rest of the sessions along with the connection.
>     
>     I think you are confusing two things: (i) notifying the application of an exception and (ii) closing the sessions and connection. I certainly don't disagree that the second is wrong. The first however is I think reasonable if that is the only way the exception can be communicated.
> 
> rajith attapattu wrote:
>     Did a quick test on the queue being deleted while a consumer is listening on.
>     We do not report that exception unless the session is being used after that for some other operation.
>
> 
> Gordon Sim wrote:
>     Fair enough, if it doesn't work properly now we wouldn't be regressing on that.
> 
> rajith attapattu wrote:
>     "I think you are confusing two things: (i) notifying the application of an exception and (ii) closing the sessions and connection. I certainly don't disagree that the second is wrong. The first however is I think reasonable if that is the only way the exception can be communicated."
>     
>     I contend that both (i) and (ii) will end up with the same result.
>     Most applications when received an exception via the Exception listener will close the connection and create a new connection.
>     It's impossible for them to know whether the connection was closed or not when they receive the exception.
> 
> Gordon Sim wrote:
>     With a clear classification of exceptions it should be possible to determine the details and scope of the error. However it sounds like you couldn't at present rely on that mechanism anyway, so the point is probably moot.

After further discussions with Rob etc.. I think we need to continue to notify session exceptions via the exception listener as it is sometimes the only way to notify an exception. A good example is the Message Listener case highlighted by Gordon.
Currently this will not work in 0-10 due to QPID-3233 - but I have a reasonable fix for this already.

Taking into account the above reasons, I think Gordon's suggestion is probably the best solution going forward.
I have kicked off some testing around this and will commit it if it doesn't seem to pose any other issues.


- rajith


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


On 2011-04-29 03:16:13, rajith attapattu wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/642/
> -----------------------------------------------------------
> 
> (Updated 2011-04-29 03:16:13)
> 
> 
> Review request for qpid.
> 
> 
> Summary
> -------
> 
> Suggested fix for https://issues.apache.org/jira/browse/QPID-3214
> Please note we may also need to fix the exceptionReceived method in AMQConnection.java for reasons outlined in the above JIRA.
> 
> 
> Diffs
> -----
> 
>   http://svn.apache.org/repos/asf/qpid/trunk/qpid/java/client/src/main/java/org/apache/qpid/client/AMQSession_0_10.java 1095508 
> 
> Diff: https://reviews.apache.org/r/642/diff
> 
> 
> Testing
> -------
> 
> 
> Thanks,
> 
> rajith
> 
>