Review Request: QPID-3911: Fix the deadlock occuring on concurrent invocation of MessageConsumer#close() and Session#rollback()

[Oleksandr Rudyy <or...@gmail.com>]

-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/4546/
-----------------------------------------------------------

Review request for qpid, Robbie Gemmell, rajith attapattu, Keith Wall, and Rob Godfrey.


Summary
-------

Invoking of MessageConsumer#close() and Session#rollback from a consumer listener results in a deadlock on 0-10 path and a timeout exception on 0-9 path

On 0-10 path the deadlock is caused by an ExecutionException sent from the broker in response to message.stop, message.flow commands (sent from Session#rollback for the suspension of the channel) when message.stop or message.flow command is delivered to the broker after message.cancel command which is sent as part of MessageConsumer#close(). Command message.cancel results in a deletion of a subscription on the broker side and for the following message.stop or message.flow command the subscription cannot be found and ExceutionException is reported:

org.apache.qpid.AMQException: ch=0 id=2 ExecutionException(errorCode=NOT_FOUND, commandId=20, classCode=4, commandCode=12, fieldIndex=0, description=not-found: Unknown destination 1 session=guest@QPID.89a222e3-e3ca-4aee-8e4f-a78468f4fed1 (qpid/broker/SemanticState.cpp:569), errorInfo={}) [error code 404: not found]

The ExecutionException is sent from both Java and C++ Brokers

On receiving an ExecutionException connection tries to acquire the FailoverMutex in order to close itself in AMQConnection#exceptionReceived method.

However, the FailoverMutex is acquired in MessageConsumer#close() which is waiting for a release of a session dispatcher lock. The last is hold in a dispatcher thread. The closing of a connection occurs in a dispatcher thread and this results in a deadlock.


The suggested patch introduces the following changes:
 Adds synchronization on AMSession#_messageDeliveryLock into MessageConsumer#close() in order to block until message listener in progress has completed(as required in JMS javadoc for MessageConsumer#close())
 Changes the session dispatcher to stop the message delivery into consumer local message queue if the consumer in the process of closing. This eliminates the need to stop the dispatcher on rejecting pending messages for closing consumer.
 Removes the synchronization on a session dispatcher lock from AMQSession.Dispatcher#rejectPending and code to stop the dispatcher as we are synchronizing on a deliveryLock now and incoming messages are not dispatched into closing consumer anymore.
 Adds a system test to reproduce the deadlock


This addresses bug QPID-3911.
    https://issues.apache.org/jira/browse/QPID-3911


Diffs
-----

  /trunk/qpid/java/client/src/main/java/org/apache/qpid/client/AMQSession.java 1306567 
  /trunk/qpid/java/client/src/main/java/org/apache/qpid/client/BasicMessageConsumer.java 1306567 
  /trunk/qpid/java/systests/src/main/java/org/apache/qpid/test/unit/close/MessageConsumerCloseTest.java PRE-CREATION 

Diff: https://reviews.apache.org/r/4546/diff


Testing
-------


Thanks,

Oleksandr