> On 2011-06-03 10:24:24, Gordon Sim wrote: > > http://svn.apache.org/repos/asf/qpid/trunk/qpid/java/client/src/main/java/org/apache/qpid/client/AMQSession_0_10.java, > > line 1033 > > <https://reviews.apache.org/r/833/diff/2/?file=20160#file20160line1033> > > > > 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. > On 2011-06-03 10:24:24, Gordon Sim wrote: > > http://svn.apache.org/repos/asf/qpid/trunk/qpid/java/common/src/main/java/org/apache/qpid/transport/Session.java, > > line 783 > > <https://reviews.apache.org/r/833/diff/2/?file=20163#file20163line783> > > > > 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. > On 2011-06-03 10:24:24, Gordon Sim wrote: > > http://svn.apache.org/repos/asf/qpid/trunk/qpid/java/common/src/main/java/org/apache/qpid/transport/SessionDelegate.java, > > line 156 > > <https://reviews.apache.org/r/833/diff/2/?file=20164#file20164line156> > > > > 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: > > ----------------------------------------------------------- > 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 > >
