----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/10738/#review20483 -----------------------------------------------------------
http://svn.apache.org/repos/asf/qpid/trunk/qpid/java/client/src/main/java/org/apache/qpid/client/AMQConnection.java <https://reviews.apache.org/r/10738/#comment42204> Whitespace http://svn.apache.org/repos/asf/qpid/trunk/qpid/java/client/src/main/java/org/apache/qpid/client/AMQConnection.java <https://reviews.apache.org/r/10738/#comment42205> Whitespace http://svn.apache.org/repos/asf/qpid/trunk/qpid/java/client/src/main/java/org/apache/qpid/client/AMQSession.java <https://reviews.apache.org/r/10738/#comment42206> Unused http://svn.apache.org/repos/asf/qpid/trunk/qpid/java/client/src/main/java/org/apache/qpid/client/AMQSession.java <https://reviews.apache.org/r/10738/#comment42207> 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. http://svn.apache.org/repos/asf/qpid/trunk/qpid/java/client/src/main/java/org/apache/qpid/client/AMQSession.java <https://reviews.apache.org/r/10738/#comment42210> 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. http://svn.apache.org/repos/asf/qpid/trunk/qpid/java/client/src/main/java/org/apache/qpid/client/AMQSession.java <https://reviews.apache.org/r/10738/#comment42211> We probably shouldn't catch all exceptions, or eat the interrupted status. http://svn.apache.org/repos/asf/qpid/trunk/qpid/java/client/src/main/java/org/apache/qpid/client/AMQSession.java <https://reviews.apache.org/r/10738/#comment42212> 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. http://svn.apache.org/repos/asf/qpid/trunk/qpid/java/client/src/main/java/org/apache/qpid/client/AMQSession.java <https://reviews.apache.org/r/10738/#comment42213> We probably shouldn't catch all exceptions, or eat the interrupted status. http://svn.apache.org/repos/asf/qpid/trunk/qpid/java/client/src/main/java/org/apache/qpid/client/BasicMessageConsumer.java <https://reviews.apache.org/r/10738/#comment42216> 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. http://svn.apache.org/repos/asf/qpid/trunk/qpid/java/client/src/main/java/org/apache/qpid/client/BasicMessageConsumer.java <https://reviews.apache.org/r/10738/#comment42217> 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? http://svn.apache.org/repos/asf/qpid/trunk/qpid/java/client/src/main/java/org/apache/qpid/client/BasicMessageConsumer.java <https://reviews.apache.org/r/10738/#comment42218> 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? http://svn.apache.org/repos/asf/qpid/trunk/qpid/java/client/src/main/java/org/apache/qpid/client/BasicMessageConsumer.java <https://reviews.apache.org/r/10738/#comment42219> Same comments as earlier about thread safety, exception handling, and reuse. http://svn.apache.org/repos/asf/qpid/trunk/qpid/java/client/src/main/java/org/apache/qpid/client/BasicMessageConsumer.java <https://reviews.apache.org/r/10738/#comment42220> Same comments as earlier about method name, thread safety, exception handling, and reuse. http://svn.apache.org/repos/asf/qpid/trunk/qpid/java/common/src/main/java/org/apache/qpid/configuration/ClientProperties.java <https://reviews.apache.org/r/10738/#comment42214> Effectively unused - Robbie Gemmell 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 > >
