Versions Compared

Key

  • This line was added.
  • This line was removed.
  • Formatting was changed.
Comment: Migrated to Confluence 5.3

...

Having the Dispatcher signal that it has stopped processing will allow us to know that we have hit the stopped state. However, this will mean that we have the opportunity to process one extra message AFTER the rollback command has been requested.

Proposed Solution

Currently there is a lot of synchronisation to ensure that we can safely start the rollback process in the AMQSession before asking the Dispatcher to clean up its resources. To ensure signal that we have stopped the dispatcher and so can guarantee we are no longer holding a message will require more synchronisation, which is both error prone and will and additional complexity to the client.

The proposed alternative is to modify the FlowControllingBlockingQueue so that we can delegate all rollback processing to the Dispatcher. This removes the need to stop the Dispatcher and if the Dispatcher is performing the rollback then it can be sure it is not currently processing an UnprocessedMessage.

While delegating the rollback of consumed messages to the Dispatcher it makes sense to give the Dispatcher more formal control over the receipt and dispatching of incoming messages. By extracting the Dispatcher from the Session class we can simplify the both classes. Locking will be reduced and the responsibility of message processing will be more cleanly delegated to the Dispatcher.

Code Block
TitleDispatcherInterface.java

/** 
 * Dispatcher is responsible for delivering messages from the IO layer to registered consumers.
 * The dispatcher provides asynchronous dispatch via Consumer MessageListeners and
 * synchronous delivery by placing incoming messages in a Consumers recieveQueue.
 */
public interface DispatcherInterface<C extends BasicMessageConsumer>
{
        /** Start this Dispatcher if required */
        public void startDispatcherIfNecessary(boolean initiallyStopped);
        /** Close this Dispatcher and release any resources held. Cannot be re-opened */
        public void close();
        /** Register a consumer to receive messages from this Dispatcher. */
        public void registerConsumer(C consumer);
        /** Unregister a consumer from this Dispatcher @return C removed consumer */
        public C unregisterConsumer(C consumer);
        /** Rollback the received message state in this Dispatcher */
        public void rollback();
        /** Process the received message from the IO Layer */
        public void dispatchMessage(UnprocessedMessage message);
}

The clean interface with the Dispatcher from the Session means that we can more clearly delegate the rollback() control to the Dispatcher. The FlowControllingBlockingQueue will need to be augmented so that when an asynchronous request for rollback is made the Dispatcher can then pick up on this 'ServiceRequest'.

Steps

...

Further Details

After a discussion with Rafi/Rob on the recent Python changes expending effort in refactoring the client is probably not worth the effort. If the client message delivery were re-written to mirror the approach taken in the Python codebase then it would be simplier and easier to reason about. As a result I have devised a much smaller, though slightly ugly approach that will address our immediate rollback issues. The approach can be found here .

Comment Responses

User

Comment

via

Response

rhs

AMQSession.syncDispatchQueue is used in 0-10 for this

email

This will not work if the dispatcher is performing the rollback (Deadlock).
Also we need to stop processing the messages immediately and not allow any further processing.

rhs

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.

email

It is not so much moving from on thread to another but from moving from the AMQSession / Dispatcher objects to just the Dispatcher.

rhs

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.

email

The focus of this change was to consolodate the operations on the received messagse. I would like to see a clean interface where messages are passed in for for dispatching. The cleaning operations should then be full contained in that interface not in a couple of locations as it is currently.

rhs

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.

email

Agreed, documenting what we have and how it works would be very useful for this discussion.