You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@qpid.apache.org by rajith attapattu <ra...@gmail.com> on 2011/04/21 00:14:56 UTC
Review Request: QPID-3216 - suggested fix for the deadlock issue.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/642/
-----------------------------------------------------------
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
Re: Review Request: QPID-3216 - suggested fix for the deadlock issue.
Posted by 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?
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.
- rajith
-----------------------------------------------------------
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
>
>
Re: Review Request: QPID-3214 - suggested fix for the deadlock issue.
Posted by 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
>
>
Re: Review Request: QPID-3216 - suggested fix for the deadlock issue.
Posted by Gordon Sim <gs...@redhat.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.
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
>
>
Re: Review Request: QPID-3216 - suggested fix for the deadlock issue.
Posted by 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.
"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.
- rajith
-----------------------------------------------------------
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
>
>
Re: Review Request: QPID-3216 - suggested fix for the deadlock issue.
Posted by Gordon Sim <gs...@redhat.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.
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
-----------------------------------------------------------
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
>
>
Re: Review Request: QPID-3216 - suggested fix for the deadlock issue.
Posted by Gordon Sim <gs...@redhat.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.
>
Fair enough, if it doesn't work properly now we wouldn't be regressing on that.
- 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
>
>
Re: Review Request: QPID-3216 - suggested fix for the deadlock issue.
Posted by 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.
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.
- rajith
-----------------------------------------------------------
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
>
>
Re: Review Request: QPID-3216 - suggested fix for the deadlock issue.
Posted by Gordon Sim <gs...@redhat.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.
> 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.
- 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
>
>
Re: Review Request: QPID-3216 - suggested fix for the deadlock issue.
Posted by Gordon Sim <gs...@redhat.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/642/#review519
-----------------------------------------------------------
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?
- Gordon
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
>
>
Re: Review Request: QPID-3214 - suggested fix for the deadlock issue.
Posted by rajith attapattu <ra...@gmail.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/642/
-----------------------------------------------------------
(Updated 2011-04-29 03:16:13.445659)
Review request for qpid.
Summary (updated)
-------
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
Re: Review Request: QPID-3216 - suggested fix for the deadlock issue.
Posted by Andrew Kennedy <an...@gmail.com>.
On 20 April 2011 23:14, rajith attapattu <ra...@gmail.com> wrote:
> Suggested fix for https://issues.apache.org/jira/browse/QPID-3216
This is for QPID-3214
Also, the 0-8 code does exactly the same thing, that is throws
exceptions to the connection listener if the session receives them, so
I'm not sure we can just ignore delivery. The problem being, if a
session fails, the only thing that can get an exception is the
connection, if it occurs asynchronously. IAs an alternative, maybe we
can remove the lock and replace it with an AtomicReference containing
the current exception, and also move the acquisition of the failover
mutex until after we have signalled the connection with the exception.
I will check how this behaves with the C++ profiles tonight.
(And, more generally, I don't view these as regressions, but as
continuing issues with locking that have been present for some
time...)
Andrew.
--
-- andrew d kennedy ? edinburgh : +44 7582 293 255
---------------------------------------------------------------------
Apache Qpid - AMQP Messaging Implementation
Project: http://qpid.apache.org
Use/Interact: mailto:dev-subscribe@qpid.apache.org