> 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.
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. - Gordon ----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/642/#review519 ----------------------------------------------------------- On 2011-04-20 22:14:56, rajith attapattu wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/642/ > ----------------------------------------------------------- > > (Updated 2011-04-20 22:14:56) > > > Review request for qpid. > > > Summary > ------- > > Suggested fix for https://issues.apache.org/jira/browse/QPID-3216 > 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 > >
