-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/10738/
-----------------------------------------------------------
(Updated May 21, 2013, 2:48 p.m.)
Review request for qpid, Robbie Gemmell, Weston Price, and Rob Godfrey.
Changes
-------
Added the missing class ConditionManager.java
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 (updated)
-----
http://svn.apache.org/repos/asf/qpid/trunk/qpid/java/client/src/main/java/org/apache/qpid/client/AMQConnection.java
1484812
http://svn.apache.org/repos/asf/qpid/trunk/qpid/java/client/src/main/java/org/apache/qpid/client/AMQSession.java
1484812
http://svn.apache.org/repos/asf/qpid/trunk/qpid/java/client/src/main/java/org/apache/qpid/client/BasicMessageConsumer.java
1484812
http://svn.apache.org/repos/asf/qpid/trunk/qpid/java/client/src/main/java/org/apache/qpid/client/BasicMessageConsumer_0_10.java
1484812
http://svn.apache.org/repos/asf/qpid/trunk/qpid/java/client/src/main/java/org/apache/qpid/client/util/ConditionManager.java
PRE-CREATION
Diff: https://reviews.apache.org/r/10738/diff/
Testing
-------
Java test suite, tests from customers and QE around the deadlock situation.
Thanks,
rajith attapattu
