> On May 13, 2013, 3:22 p.m., Robbie Gemmell wrote: > > http://svn.apache.org/repos/asf/qpid/trunk/qpid/java/client/src/main/java/org/apache/qpid/client/AMQSession.java, > > lines 255-256 > > <https://reviews.apache.org/r/10738/diff/2/?file=288992#file288992line255> > > > > Unused
Will mark for removal. > On May 13, 2013, 3:22 p.m., Robbie Gemmell wrote: > > http://svn.apache.org/repos/asf/qpid/trunk/qpid/java/client/src/main/java/org/apache/qpid/client/AMQSession.java, > > line 750 > > <https://reviews.apache.org/r/10738/diff/2/?file=288992#file288992line750> > > > > Might be better called 'waitForInProgressDeliveryToComplete' as it > > doesn't actually wait for all message delivery to finish (although that > > might be the case, depending on other factors outside its control) but > > rather any that is already in progress to complete. Make sense, I will make that change. > On May 13, 2013, 3:22 p.m., Robbie Gemmell wrote: > > http://svn.apache.org/repos/asf/qpid/trunk/qpid/java/client/src/main/java/org/apache/qpid/client/AMQSession.java, > > lines 758-766 > > <https://reviews.apache.org/r/10738/diff/2/?file=288992#file288992line758> > > > > We should hold the object lock while performing the check, otherwise we > > could go into the wait() after the only notify() that could wake us has > > already been performed. Good catch! > On May 13, 2013, 3:22 p.m., Robbie Gemmell wrote: > > http://svn.apache.org/repos/asf/qpid/trunk/qpid/java/client/src/main/java/org/apache/qpid/client/AMQSession.java, > > line 767 > > <https://reviews.apache.org/r/10738/diff/2/?file=288992#file288992line767> > > > > We probably shouldn't catch all exceptions, or eat the interrupted > > status. For starters I will narrow it down to interrupted exception and then log it. > On May 13, 2013, 3:22 p.m., Robbie Gemmell wrote: > > http://svn.apache.org/repos/asf/qpid/trunk/qpid/java/client/src/main/java/org/apache/qpid/client/AMQSession.java, > > lines 3339-3347 > > <https://reviews.apache.org/r/10738/diff/2/?file=288992#file288992line3339> > > > > Linked to earlier comment, we need to hold the object lock while > > performing the update or we could potentially notify() before they call > > wait(), which might mean they never wake up. Will fix this > On May 13, 2013, 3:22 p.m., Robbie Gemmell wrote: > > http://svn.apache.org/repos/asf/qpid/trunk/qpid/java/client/src/main/java/org/apache/qpid/client/BasicMessageConsumer.java, > > line 137 > > <https://reviews.apache.org/r/10738/diff/2/?file=288993#file288993line137> > > > > This and all the code that uses it seems to duplicate the code added in > > AMQSession. It seems like this stuff could go in a utility class to avoid > > the duplication, which would also permit easy addition of some unit tests. Sure, will do that. > On May 13, 2013, 3:22 p.m., Robbie Gemmell wrote: > > http://svn.apache.org/repos/asf/qpid/trunk/qpid/java/client/src/main/java/org/apache/qpid/client/BasicMessageConsumer.java, > > lines 771-785 > > <https://reviews.apache.org/r/10738/diff/2/?file=288993#file288993line771> > > > > Same comments as earlier about thread safety, exception handling, and > > reuse. Will be addressed in next rev. > On May 13, 2013, 3:22 p.m., Robbie Gemmell wrote: > > http://svn.apache.org/repos/asf/qpid/trunk/qpid/java/client/src/main/java/org/apache/qpid/client/BasicMessageConsumer.java, > > lines 1059-1082 > > <https://reviews.apache.org/r/10738/diff/2/?file=288993#file288993line1059> > > > > Same comments as earlier about method name, thread safety, exception > > handling, and reuse. Will be addresed in next rev. > On May 13, 2013, 3:22 p.m., Robbie Gemmell wrote: > > http://svn.apache.org/repos/asf/qpid/trunk/qpid/java/client/src/main/java/org/apache/qpid/client/AMQConnection.java, > > line 873 > > <https://reviews.apache.org/r/10738/diff/2/?file=288991#file288991line873> > > > > Whitespace Will fix this. Can't see this in my git patch. (The diff for RB is created from my svn checkout, as RB doesn't like the git formatted patch). Will double check all whitespace issues before I commit. > On May 13, 2013, 3:22 p.m., Robbie Gemmell wrote: > > http://svn.apache.org/repos/asf/qpid/trunk/qpid/java/client/src/main/java/org/apache/qpid/client/BasicMessageConsumer.java, > > lines 744-745 > > <https://reviews.apache.org/r/10738/diff/2/?file=288993#file288993line744> > > > > What impact does dropping the message on the floor here have on the > > client? > > > > Earlier points in the call hierarchy seem to make effort to do other > > things with the message when detecting session/consumer close, so is there > > any impact from not doing so? E.g a message getting stuck acquired for a > > now-closed consumer? > > > > Does any particular attention need paid to the overridden 0-10 specific > > version of this method? The message will be dropped if a consumer (or session is closed or closing). When a consumer is closed, any messages acquired but not acknowledged should be be made available to another consumer by the broker. These messages will be marked redelivered. Is this the same situation for 0-8/0-9 ? The situation is the same as a consumer closing with a bunch of unacked messages when in CLIENT_ACK mode. Alternatively we could reject the message (release in 0-10 terms). But I don't think this is required, given that we will be closing the consumer anyways. >Earlier points in the call hierarchy seem to make effort to do other things >with the message when detecting session/consumer close By "other things" are you referring to a reject? In 0-10 AFIK you don't need to do it. The situation is the same as a consumer closing with a bunch of unacked messages when in CLIENT_ACK mode. >Does any particular attention need paid to the overridden 0-10 specific >version of this method? IMO adding "if (!(isClosed() || isClosing()))" is required to prevent the 0-10 specific method from issuing credit (and receiving more messages, that will eventually get released). There is a chance where the consumer could be marked closed, but not yet reached the point of sending a cancel, by the time messageFlow() is called. The above check should prevent it. > On May 13, 2013, 3:22 p.m., Robbie Gemmell wrote: > > http://svn.apache.org/repos/asf/qpid/trunk/qpid/java/client/src/main/java/org/apache/qpid/client/BasicMessageConsumer.java, > > lines 594-595 > > <https://reviews.apache.org/r/10738/diff/2/?file=288993#file288993line594> > > > > Removal of this usage of the old lock will cause a fair shift in client > > behaviour, allowing the consumer close to proceed at the same time as > > message delivery is ongoing on the session, possibly entailing things such > > as the Dispatcher performing a session rollback in a message listener while > > this close is in progress. > > > > How clear are we on what impact this change has on the client? For > > example, this lock usage was apparently added specifically to prevent a > > deadlock in that sort of situation. Has your investigation of this change > > in behaviour determined whether that would become a problem again? Did you mean race condition (instead of deadlock)? The Consumer will check if it's closed (or closing) before it tries to deliver the message to the application. Therefore even if the session initiates a delivery before the consumer is marked closed it will not proceed beyond the notifyConsumer method in BasicMessageConsumer. The dropped message will be released when the consumer is closed. >Dispatcher performing a session rollback in a message listener while this >close is in progress. AFAIK the rollback method did not grab the _messageDeliveryLock, instead it was relying on "_lock". Both close() and notifyMessage() hold this lock for the entire duration of the call, thereby keeping these operations mutually exclusive. Perhaps I misunderstood your concern here? If so please explain again. - rajith ----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/10738/#review20483 ----------------------------------------------------------- On May 8, 2013, 2:02 p.m., rajith attapattu wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/10738/ > ----------------------------------------------------------- > > (Updated May 8, 2013, 2:02 p.m.) > > > Review request for qpid, Robbie Gemmell, Weston Price, and Rob Godfrey. > > > Description > ------- > > There are at least 3 cases where the deadlock btw _messageDeliveryLock and > _failoverMutex surfaces. Among them sending a message inside onMessage() and > the session being closed due to an error (causing the deadlock) seems to come > up a lot in production environments. There is also a deadlock btw > _messageDeliveryLock and _lock (AMQSession.java) which happens less > frequently. > The messageDeliveryLock is used to ensure that we don't close the session in > the middle of a message delivery. In order to do this we hold the lock across > onMessage(). > This causes several issues in addition to the potential to deadlock. If an > onMessage call takes longer/wedged then you cannot close the session or > failover will not happen until it returns as the same thread is holding the > failoverMutex. > > Based on an idea from Rafi, I have come up with a solution to get rid of > _messageDeliveryLock and instead use an alternative strategy to achieve > similar functionality. > In order to ensure that close() doesn't proceed until the message deliveries > currently in progress completes, an atomic counter is used to keep track of > message deliveries in progress. > The close() will wait until the count falls to zero before proceeding. No new > deliveries will be initiated bcos the close method will mark the session as > closed. > The wait has a timeout to ensure that a longer running or wedged onMessage() > will not hold up session close. > There is a slim chance that before a session being marked as closed a message > delivery could be initiated, but not yet gotten to the point of updating the > counter, hence waitForMsgDeliveryToFinish() will see it as zero and proceed > with close. But in comparison to the issues with _messageDeliveryLock, I > believe it's acceptable. > > There is an issue if MessageConsumer close is called outside of Session > close. This can be solved in a similar manner. I will wait until the current > review is complete and then post the solution for the MessageConsumer close. > I will commit them both together. > > > This addresses bug QPID-4574. > https://issues.apache.org/jira/browse/QPID-4574 > > > Diffs > ----- > > > http://svn.apache.org/repos/asf/qpid/trunk/qpid/java/client/src/main/java/org/apache/qpid/client/AMQConnection.java > 1480271 > > http://svn.apache.org/repos/asf/qpid/trunk/qpid/java/client/src/main/java/org/apache/qpid/client/AMQSession.java > 1480271 > > http://svn.apache.org/repos/asf/qpid/trunk/qpid/java/client/src/main/java/org/apache/qpid/client/BasicMessageConsumer.java > 1480271 > > http://svn.apache.org/repos/asf/qpid/trunk/qpid/java/common/src/main/java/org/apache/qpid/configuration/ClientProperties.java > 1480271 > > Diff: https://reviews.apache.org/r/10738/diff/ > > > Testing > ------- > > Java test suite, tests from customers and QE around the deadlock situation. > > > Thanks, > > rajith attapattu > >
