Martin Ritchie wrote:
Hi Rafi,
I saw the syncDispatchQueue method but I don't think waiting for the
_queue to clear is not the right solution, IMO, for rollback, even on
0-10. When rollback is called you don't want the dispatcher to process
any more messages. Your client may have MessageListeners setup that
will take a long time to process, so the Dispatcher should stop
processing messages and perform the Rollback.
I've attached a new test for RollbackOrderTest that blocks because the
syncDispatchQueue waits for the Dispatcher to empty the _queue.
However, when called via the MessageListener.onMessage(), you end up
blocking the Dispatcher.
That's a good point, however won't this also be an issue in the design
you're proposing? Regardless of which thread actually does the rollback
processing, rollback needs to block until that processing is complete,
and if you're waiting for the "ServiceRequest" to be processed from the
thread that is supposed to process it then you have essentially the same
deadlock you've attached below.
I think extracting the Dispatcher will make it clearer to show that
the message processing varies in each protocol and will allow the
Session classes to focus on the creation and control of
Consumer/Producers. This will allow Dispatchers for each protocol to
be cleaner and highlight the protocol differences at failover; A 0-8
Dispatcher that simply drops all pending messages compared to the 0-10
Dispatcher that attempts to process all the messages it has received.
I don't actually think any of this needs to be (or should be) protocol
specific. AFAICT there's no relevant difference in the protocol
semantics here, these issues are common to any protocol that does
prefetch. For example, the choice of whether to drop or process pending
messages in a given scenario could be made either way for any protocol
version.
I think it is a significant change but I think it is worth it as it
will improve the readability of the Client code as well as improving
the testability. Currently AMQSession is not exactly easy to unit test
so splitting it in to more discrete components and unit testing them
will be beneficial.
The change boils down to:
- Extract Dispatcher Logic to Dispatcher_0_8 , Dispatcher_0_10
- Update AMQSession to use new Dispatcher
- Update Dispatcher to be able to stop processing the _queue of
messages and perform rollback.
I agree the client is badly in need of some improvements in
maintainability and readability, however in this particular case I don't
think moving the rollback processing from one thread to another actually
improves the situation significantly. Fundamentally the rollback logic
needs to be performed from at least two different threads, the
dispatcher thread and the application thread. I suspect in order do this
properly we really need to stop thinking in terms of code being
associated with a given thread, and think instead about what locks we
have, what data structures those locks protect, and which locks need to
be held in order to execute a given piece of code.
In my experience the number one issue with the client is deadlocks and
race conditions stemming from the large number of haphazardly defined
locks/conditions that are littered throughout the client code. I think
before we can safely and productively move large chunks of code around
we really need to have some sort plan for documenting and simplifying
the locking situation. Really we need to be able to articulate exactly
what locks the client has, what data structure(s) each lock protects,
and what order should be used to acquire multiple locks when necessary.
--Rafael
---------------------------------------------------------------------
Apache Qpid - AMQP Messaging Implementation
Project: http://qpid.apache.org
Use/Interact: mailto:dev-subscr...@qpid.apache.org
