----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/10738/ -----------------------------------------------------------
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. 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 1471133 http://svn.apache.org/repos/asf/qpid/trunk/qpid/java/client/src/main/java/org/apache/qpid/client/AMQSession.java 1471133 http://svn.apache.org/repos/asf/qpid/trunk/qpid/java/client/src/main/java/org/apache/qpid/client/BasicMessageConsumer.java 1471133 http://svn.apache.org/repos/asf/qpid/trunk/qpid/java/common/src/main/java/org/apache/qpid/configuration/ClientProperties.java 1471133 Diff: https://reviews.apache.org/r/10738/diff/ Testing ------- Java test suite, tests from customers and QE around the deadlock situation. Thanks, rajith attapattu