> On April 24, 2013, 4:24 p.m., Robbie Gemmell wrote: > > I don't really agree that there is only a slim chance a delivery can begin > > before the session is marked closed, it actually seems fairly likely to > > occur (unless none of the queues used by the consumers on the session have > > any messages left, like most of our tests). This could lead to a variety of > > issues as a result, because important portions of the client depend on > > being able to guarantee no more messages are being put into the consumer > > receive queue or onMessage() callbacks fired while they are operating. I > > don't get the impression these have been investigated enough to prove their > > correctness in the face of this change, so it doesn't seem particularly > > acceptable a trade-off as it stands. > > > > It seems you have already noticed that there would be issues with the > > consumer close, since it would no longer stop deliveries occurring while > > closing was in progress, making it one of the cases mentioned above. > > > > Given that msgDeliveriesInProgress is per-session and only > > incremented/decremented by the Dispatcher thread in a single place, > > shouldn't it just be an AtomicBoolean? > > rajith attapattu wrote: > For the above case, the following needs to happen. > > 1. A delivery is started just before session close is called, but haven't > got to the point where the count is incremented, by the time the close method > calls waitForMsgDeliveryToFinish(). > 2. There are no outstanding deliveries at the time > waitForMsgDeliveryToFinish() looks at "_msgDeliveriesInProgress". > > That is why I think there is a "slim" chance. If there are messages left > on the queue, then there is reasonable chance that _msgDeliveriesInProgress > is not zero. So the waitForMsgDeliveryToFinish() will block anyways. > I'm currently working on plugging this gap. Provided this is taken care > of do you see any holes in this approach ? > > As for why I think this is an acceptable trade-off, most customers prefer > this loop hole over a client that deadlocks every now and then. > This deadlock has been around for a long time with no solution and there > are several customers who are experiencing this deadlock in their production > environment. > > "I don't get the impression these have been investigated enough to prove > their correctness in the face of this change, so it doesn't seem particularly > acceptable a trade-off as it stands." -- There is ample evidence that the > current setup is causing several deadlocks to the point of our client being > useless. This solution (baring the above issue) passes all the tests, > including a trial run at customer environments. > What would you deem acceptable (provided the current issue above is > resolved) for you to gain confidence for the proposed fix ? > > Do you have an alternative solution? Or a way to improve the current one. > I'm sure we can find a way if we put our heads together. > The bottom line is this issue needs to be resolved, or else people will > loose confidence in our client.
"It seems you have already noticed that there would be issues with the consumer close, since it would no longer stop deliveries occurring while closing was in progress, making it one of the cases mentioned above." -- This was identified at the outset, but wasn't included in this to ensure this aspect of the solution is highlighted and debated first. I'm currently working on a solution to this, which I'm hoping will also serve as a solution to the problem that's used in AMQSession. Again suggestions and ideas are most welcomed. - rajith ----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/10738/#review19626 ----------------------------------------------------------- On April 24, 2013, 12:19 p.m., rajith attapattu wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/10738/ > ----------------------------------------------------------- > > (Updated April 24, 2013, 12:19 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 > 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 > >
