-----------------------------------------------------------
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.
Changes
-------
The updated patch address the following in addition to the previous patch,
1. Prevents message delivery if the session is marked for close.
2. Prevents message delivery if the Consumer is marked for close.
3. In both session and consumer, now there is no chance a session/consumer
would be closed while a delivery is in progress.
4. Removed the timeout in waitForMsgDeliveryToFinish. Now the method will block
until a delivery completes (as per the JMS spec).
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
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
