----------------------------------------------------------- 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: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/833/ > ----------------------------------------------------------- > > (Updated 2011-06-02 02:52:57) > > > 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 > >
