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