[
https://issues.apache.org/jira/browse/QPID-3289?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=13043424#comment-13043424
]
[email protected] 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:[email protected]
