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 (JIRA)" <ji...@apache.org> on 2011/06/02 04:38:47 UTC
[jira] [Created] (QPID-3289) Session exceptions should only be
notified via the exception listener, if it cannot be thrown directly to the
application.
Session exceptions should only be notified via the exception listener, if it cannot be thrown directly to the application.
--------------------------------------------------------------------------------------------------------------------------
Key: QPID-3289
URL: https://issues.apache.org/jira/browse/QPID-3289
Project: Qpid
Issue Type: Bug
Reporter: Rajith Attapattu
Assignee: Rajith Attapattu
The 0-10 code path in the JMS client always notifies a session exception via the connection listener, even when it can throw a JMS exception for a synchronous method call.
For example session.createConsumer(destination) could fail due to an ACL violation and the session gets closed with an execution exception.
Currently the JMS client throws an exception and also notifies the connection listener which results in the connection (and any other sessions associated with that connection) being closed.
Another undesirable effect of this is the potential for deadlocks happening around the failovermutex.
A reasonable solution for this problem would be to not notify via the connection listener if there is a way of throwing an exception to the application directly.
For example session.createConsumer/Producer is a synchronous call and we could easily throw a JMS exception. In this case there is little value in notifying this via the connection listener and it also has the undesirable effect of closing the connection.
--
This message is automatically generated by JIRA.
For more information on JIRA, see: http://www.atlassian.com/software/jira
---------------------------------------------------------------------
Apache Qpid - AMQP Messaging Implementation
Project: http://qpid.apache.org
Use/Interact: mailto:dev-subscribe@qpid.apache.org
[jira] [Commented] (QPID-3289) Session exceptions should only be
notified via the exception listener, if it cannot be thrown directly to the
application.
Posted by "jiraposter@reviews.apache.org (JIRA)" <ji...@apache.org>.
[ https://issues.apache.org/jira/browse/QPID-3289?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=13043424#comment-13043424 ]
jiraposter@reviews.apache.org commented on QPID-3289:
-----------------------------------------------------
bq. On 2011-06-03 10:24:24, Gordon Sim wrote:
bq. > http://svn.apache.org/repos/asf/qpid/trunk/qpid/java/client/src/main/java/org/apache/qpid/client/AMQSession_0_10.java, line 1033
bq. > <https://reviews.apache.org/r/833/diff/2/?file=20160#file20160line1033>
bq. >
bq. > Using the setter but not the getter for a variable protected by its own lock looks odd and at least deserves a comment as to why it is done this way and why it is safe.
Good question. The getCurrentException method will set the _currentException to null once it returns the exception. So a subsequent call to a method in the session will throw a session closed exception with the underlying cause being empty (due to _currentException) being null.
One way to avoid this is to modify the getCurrentException method to not null the _currentException variable, but not sure what impact it might have.
As for safety, I don't think the broker will send any other session exception after the session is invalidated. So in that sense I think it's safe to use the variable directly.
bq. On 2011-06-03 10:24:24, Gordon Sim wrote:
bq. > http://svn.apache.org/repos/asf/qpid/trunk/qpid/java/common/src/main/java/org/apache/qpid/transport/Session.java, line 783
bq. > <https://reviews.apache.org/r/833/diff/2/?file=20163#file20163line783>
bq. >
bq. > Minor point, but this patch adds several new cases of spurious trailing whitespace. In this case it even adds to the noise and distracts from the real changes.
Yes this is annoying. Eclipse is adding these. I will try to take care of this.
bq. On 2011-06-03 10:24:24, Gordon Sim wrote:
bq. > http://svn.apache.org/repos/asf/qpid/trunk/qpid/java/common/src/main/java/org/apache/qpid/transport/SessionDelegate.java, line 156
bq. > <https://reviews.apache.org/r/833/diff/2/?file=20164#file20164line156>
bq. >
bq. > A bit more explanation of the purpose of this check would be helpful. Is it envisaged to be called concurrent to some other call to sync()? Or is this detecting whether we are within a sync on the current thread? In the former case you would seem to have a race condition which may or may not be ok. In the latter there might be a nicer way to express it.
It's the former - the Session.sync() and SessionDelegate.executionException() methods can be called concurrently and it can indeed cause problems.
There are a few corner cases that can cause problems. I will have to rethink this section.
- rajith
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/833/#review752
-----------------------------------------------------------
On 2011-06-02 02:52:57, rajith attapattu wrote:
bq.
bq. -----------------------------------------------------------
bq. This is an automatically generated e-mail. To reply, visit:
bq. https://reviews.apache.org/r/833/
bq. -----------------------------------------------------------
bq.
bq. (Updated 2011-06-02 02:52:57)
bq.
bq.
bq. Review request for qpid.
bq.
bq.
bq. Summary
bq. -------
bq.
bq. As mentioned in the JIRA a simple solution is to not notify via the exception listener if a JMS exception can be directly thrown to the application.
bq. The attached patch (unpolished) makes use of the sync call to determine if an application can be notified directly or not.
bq. If it is then it will not notify via the connection listener.
bq. Andrew Kennedy had done some work previously to introduce a sync method in AMQSession_0_10.java to facilitate sync calls.
bq. This patch builds on top of it to provide the above functionality.
bq.
bq. This patch also fixes issues like QPID-3259 and avoids other potential locking issues due to the two ways(paths) of notifying the same error.
bq. Please note this patch only affects the 0-10 code path.
bq.
bq.
bq. This addresses bug QPID-3289.
bq. https://issues.apache.org/jira/browse/QPID-3289
bq.
bq.
bq. Diffs
bq. -----
bq.
bq. http://svn.apache.org/repos/asf/qpid/trunk/qpid/java/client/src/main/java/org/apache/qpid/client/AMQSession_0_10.java 1129752
bq. http://svn.apache.org/repos/asf/qpid/trunk/qpid/java/client/src/main/java/org/apache/qpid/client/BasicMessageConsumer_0_10.java 1129752
bq. http://svn.apache.org/repos/asf/qpid/trunk/qpid/java/client/src/main/java/org/apache/qpid/client/BasicMessageProducer_0_10.java 1129752
bq. http://svn.apache.org/repos/asf/qpid/trunk/qpid/java/common/src/main/java/org/apache/qpid/transport/Session.java 1129752
bq. http://svn.apache.org/repos/asf/qpid/trunk/qpid/java/common/src/main/java/org/apache/qpid/transport/SessionDelegate.java 1129752
bq.
bq. Diff: https://reviews.apache.org/r/833/diff
bq.
bq.
bq. Testing
bq. -------
bq.
bq.
bq. Thanks,
bq.
bq. rajith
bq.
bq.
> Session exceptions should only be notified via the exception listener, if it cannot be thrown directly to the application.
> --------------------------------------------------------------------------------------------------------------------------
>
> Key: QPID-3289
> URL: https://issues.apache.org/jira/browse/QPID-3289
> Project: Qpid
> Issue Type: Bug
> Reporter: Rajith Attapattu
> Assignee: Rajith Attapattu
>
> The 0-10 code path in the JMS client always notifies a session exception via the connection listener, even when it can throw a JMS exception for a synchronous method call.
> For example session.createConsumer(destination) could fail due to an ACL violation and the session gets closed with an execution exception.
> Currently the JMS client throws an exception and also notifies the connection listener which results in the connection (and any other sessions associated with that connection) being closed.
> Another undesirable effect of this is the potential for deadlocks happening around the failovermutex.
> A reasonable solution for this problem would be to not notify via the connection listener if there is a way of throwing an exception to the application directly.
> For example session.createConsumer/Producer is a synchronous call and we could easily throw a JMS exception. In this case there is little value in notifying this via the connection listener and it also has the undesirable effect of closing the connection.
--
This message is automatically generated by JIRA.
For more information on JIRA, see: http://www.atlassian.com/software/jira
---------------------------------------------------------------------
Apache Qpid - AMQP Messaging Implementation
Project: http://qpid.apache.org
Use/Interact: mailto:dev-subscribe@qpid.apache.org
[jira] [Commented] (QPID-3289) Session exceptions should only be
notified via the exception listener, if it cannot be thrown directly to the
application.
Posted by "jiraposter@reviews.apache.org (JIRA)" <ji...@apache.org>.
[ https://issues.apache.org/jira/browse/QPID-3289?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=13042580#comment-13042580 ]
jiraposter@reviews.apache.org commented on QPID-3289:
-----------------------------------------------------
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/833/
-----------------------------------------------------------
Review request for qpid.
Summary
-------
As mentioned in the JIRA a simple solution is to not notify via the exception listener if a JMS exception can be directly thrown to the application.
The attached patch (unpolished) makes use of the sync call to determine if an application can be notified directly or not.
If it is then it will not notify via the connection listener.
Andrew Kennedy had done some work previously to introduce a sync method in AMQSession_0_10.java to facilitate sync calls.
This patch builds on top of it to provide the above functionality.
This patch also fixes issues like QPID-3259 and avoids other potential locking issues due to the two ways(paths) of notifying the same error.
Please note this patch only affects the 0-10 code path.
This addresses bug QPID-3289.
https://issues.apache.org/jira/browse/QPID-3289
Diffs
-----
http://svn.apache.org/repos/asf/qpid/trunk/qpid/java/client/src/main/java/org/apache/qpid/client/AMQSession_0_10.java 1129752
http://svn.apache.org/repos/asf/qpid/trunk/qpid/java/client/src/main/java/org/apache/qpid/client/BasicMessageConsumer_0_10.java 1129752
http://svn.apache.org/repos/asf/qpid/trunk/qpid/java/client/src/main/java/org/apache/qpid/client/BasicMessageProducer_0_10.java 1129752
http://svn.apache.org/repos/asf/qpid/trunk/qpid/java/common/src/main/java/org/apache/qpid/transport/Session.java 1129752
http://svn.apache.org/repos/asf/qpid/trunk/qpid/java/common/src/main/java/org/apache/qpid/transport/SessionDelegate.java 1129752
Diff: https://reviews.apache.org/r/833/diff
Testing
-------
Thanks,
rajith
> Session exceptions should only be notified via the exception listener, if it cannot be thrown directly to the application.
> --------------------------------------------------------------------------------------------------------------------------
>
> Key: QPID-3289
> URL: https://issues.apache.org/jira/browse/QPID-3289
> Project: Qpid
> Issue Type: Bug
> Reporter: Rajith Attapattu
> Assignee: Rajith Attapattu
>
> The 0-10 code path in the JMS client always notifies a session exception via the connection listener, even when it can throw a JMS exception for a synchronous method call.
> For example session.createConsumer(destination) could fail due to an ACL violation and the session gets closed with an execution exception.
> Currently the JMS client throws an exception and also notifies the connection listener which results in the connection (and any other sessions associated with that connection) being closed.
> Another undesirable effect of this is the potential for deadlocks happening around the failovermutex.
> A reasonable solution for this problem would be to not notify via the connection listener if there is a way of throwing an exception to the application directly.
> For example session.createConsumer/Producer is a synchronous call and we could easily throw a JMS exception. In this case there is little value in notifying this via the connection listener and it also has the undesirable effect of closing the connection.
--
This message is automatically generated by JIRA.
For more information on JIRA, see: http://www.atlassian.com/software/jira
---------------------------------------------------------------------
Apache Qpid - AMQP Messaging Implementation
Project: http://qpid.apache.org
Use/Interact: mailto:dev-subscribe@qpid.apache.org
[jira] [Updated] (QPID-3289) Session exceptions should only be
notified via the exception listener, if it cannot be thrown directly to the
application.
Posted by "Robbie Gemmell (Updated) (JIRA)" <ji...@apache.org>.
[ https://issues.apache.org/jira/browse/QPID-3289?page=com.atlassian.jira.plugin.system.issuetabpanels:all-tabpanel ]
Robbie Gemmell updated QPID-3289:
---------------------------------
Component/s: Java Client
> Session exceptions should only be notified via the exception listener, if it cannot be thrown directly to the application.
> --------------------------------------------------------------------------------------------------------------------------
>
> Key: QPID-3289
> URL: https://issues.apache.org/jira/browse/QPID-3289
> Project: Qpid
> Issue Type: Bug
> Components: Java Client
> Reporter: Rajith Attapattu
> Assignee: Rajith Attapattu
>
> The 0-10 code path in the JMS client always notifies a session exception via the connection listener, even when it can throw a JMS exception for a synchronous method call.
> For example session.createConsumer(destination) could fail due to an ACL violation and the session gets closed with an execution exception.
> Currently the JMS client throws an exception and also notifies the connection listener which results in the connection (and any other sessions associated with that connection) being closed.
> Another undesirable effect of this is the potential for deadlocks happening around the failovermutex.
> A reasonable solution for this problem would be to not notify via the connection listener if there is a way of throwing an exception to the application directly.
> For example session.createConsumer/Producer is a synchronous call and we could easily throw a JMS exception. In this case there is little value in notifying this via the connection listener and it also has the undesirable effect of closing the connection.
--
This message is automatically generated by JIRA.
If you think it was sent incorrectly, please contact your JIRA administrators: https://issues.apache.org/jira/secure/ContactAdministrators!default.jspa
For more information on JIRA, see: http://www.atlassian.com/software/jira
---------------------------------------------------------------------
Apache Qpid - AMQP Messaging Implementation
Project: http://qpid.apache.org
Use/Interact: mailto:dev-subscribe@qpid.apache.org
[jira] [Commented] (QPID-3289) Session exceptions should only be
notified via the exception listener, if it cannot be thrown directly to the
application.
Posted by "jiraposter@reviews.apache.org (JIRA)" <ji...@apache.org>.
[ https://issues.apache.org/jira/browse/QPID-3289?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=13043284#comment-13043284 ]
jiraposter@reviews.apache.org commented on QPID-3289:
-----------------------------------------------------
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/833/#review752
-----------------------------------------------------------
http://svn.apache.org/repos/asf/qpid/trunk/qpid/java/client/src/main/java/org/apache/qpid/client/AMQSession_0_10.java
<https://reviews.apache.org/r/833/#comment1567>
Using the setter but not the getter for a variable protected by its own lock looks odd and at least deserves a comment as to why it is done this way and why it is safe.
http://svn.apache.org/repos/asf/qpid/trunk/qpid/java/common/src/main/java/org/apache/qpid/transport/Session.java
<https://reviews.apache.org/r/833/#comment1568>
Minor point, but this patch adds several new cases of spurious trailing whitespace. In this case it even adds to the noise and distracts from the real changes.
http://svn.apache.org/repos/asf/qpid/trunk/qpid/java/common/src/main/java/org/apache/qpid/transport/Session.java
<https://reviews.apache.org/r/833/#comment1569>
The updating of the waitingOnSync flag above happens under a lock. Is this supposed to be unlocked here?
http://svn.apache.org/repos/asf/qpid/trunk/qpid/java/common/src/main/java/org/apache/qpid/transport/SessionDelegate.java
<https://reviews.apache.org/r/833/#comment1570>
A bit more explanation of the purpose of this check would be helpful. Is it envisaged to be called concurrent to some other call to sync()? Or is this detecting whether we are within a sync on the current thread? In the former case you would seem to have a race condition which may or may not be ok. In the latter there might be a nicer way to express it.
- Gordon
On 2011-06-02 02:52:57, rajith attapattu wrote:
bq.
bq. -----------------------------------------------------------
bq. This is an automatically generated e-mail. To reply, visit:
bq. https://reviews.apache.org/r/833/
bq. -----------------------------------------------------------
bq.
bq. (Updated 2011-06-02 02:52:57)
bq.
bq.
bq. Review request for qpid.
bq.
bq.
bq. Summary
bq. -------
bq.
bq. As mentioned in the JIRA a simple solution is to not notify via the exception listener if a JMS exception can be directly thrown to the application.
bq. The attached patch (unpolished) makes use of the sync call to determine if an application can be notified directly or not.
bq. If it is then it will not notify via the connection listener.
bq. Andrew Kennedy had done some work previously to introduce a sync method in AMQSession_0_10.java to facilitate sync calls.
bq. This patch builds on top of it to provide the above functionality.
bq.
bq. This patch also fixes issues like QPID-3259 and avoids other potential locking issues due to the two ways(paths) of notifying the same error.
bq. Please note this patch only affects the 0-10 code path.
bq.
bq.
bq. This addresses bug QPID-3289.
bq. https://issues.apache.org/jira/browse/QPID-3289
bq.
bq.
bq. Diffs
bq. -----
bq.
bq. http://svn.apache.org/repos/asf/qpid/trunk/qpid/java/client/src/main/java/org/apache/qpid/client/AMQSession_0_10.java 1129752
bq. http://svn.apache.org/repos/asf/qpid/trunk/qpid/java/client/src/main/java/org/apache/qpid/client/BasicMessageConsumer_0_10.java 1129752
bq. http://svn.apache.org/repos/asf/qpid/trunk/qpid/java/client/src/main/java/org/apache/qpid/client/BasicMessageProducer_0_10.java 1129752
bq. http://svn.apache.org/repos/asf/qpid/trunk/qpid/java/common/src/main/java/org/apache/qpid/transport/Session.java 1129752
bq. http://svn.apache.org/repos/asf/qpid/trunk/qpid/java/common/src/main/java/org/apache/qpid/transport/SessionDelegate.java 1129752
bq.
bq. Diff: https://reviews.apache.org/r/833/diff
bq.
bq.
bq. Testing
bq. -------
bq.
bq.
bq. Thanks,
bq.
bq. rajith
bq.
bq.
> Session exceptions should only be notified via the exception listener, if it cannot be thrown directly to the application.
> --------------------------------------------------------------------------------------------------------------------------
>
> Key: QPID-3289
> URL: https://issues.apache.org/jira/browse/QPID-3289
> Project: Qpid
> Issue Type: Bug
> Reporter: Rajith Attapattu
> Assignee: Rajith Attapattu
>
> The 0-10 code path in the JMS client always notifies a session exception via the connection listener, even when it can throw a JMS exception for a synchronous method call.
> For example session.createConsumer(destination) could fail due to an ACL violation and the session gets closed with an execution exception.
> Currently the JMS client throws an exception and also notifies the connection listener which results in the connection (and any other sessions associated with that connection) being closed.
> Another undesirable effect of this is the potential for deadlocks happening around the failovermutex.
> A reasonable solution for this problem would be to not notify via the connection listener if there is a way of throwing an exception to the application directly.
> For example session.createConsumer/Producer is a synchronous call and we could easily throw a JMS exception. In this case there is little value in notifying this via the connection listener and it also has the undesirable effect of closing the connection.
--
This message is automatically generated by JIRA.
For more information on JIRA, see: http://www.atlassian.com/software/jira
---------------------------------------------------------------------
Apache Qpid - AMQP Messaging Implementation
Project: http://qpid.apache.org
Use/Interact: mailto:dev-subscribe@qpid.apache.org