Issue Details (XML | Word | Printable)

Key: MULE-3302
Type: Bug Bug
Status: Reopened Reopened
Priority: Major Major
Assignee: Ross Mason
Reporter: Ross Mason
Votes: 0
Watchers: 0
Operations

If you were logged in you would be able to see more operations.
Mule

We still get Message scribbling under certain conditions (Request Response) (It gets caught but we need to remove the underlying probblem)

Created: 05/May/08 11:36 AM   Updated: 12/Nov/08 08:58 PM
Component/s: Core: Concurrency / Threading
Affects Version/s: 2.0.1
Fix Version/s: Tech. Debt

Time Tracking:
Issue & Sub-Tasks
Issue Only
Not Specified

Issue Links:
Related
 

Labels: 20-messages
User impact: Medium
Effort points: 10

Sub-Tasks  All   Open   

 Description  « Hide
From Dan D.
I was getting slightly annoyed by the build server today so I tracked down a reliable way to reproduce this issue. To reproduce this issue locally just add a Thread.sleep(1000) to VMMessageRequester at line 89 - right after the resetAccessControl().

I think I can even explain the issue here. We have thread #1 which sends off a message which is in essence "new DefaultMuleMessage(new DefaultMuleMessageAdapter());"

Thread #2 (i.e. the main thread in the test case), picks this up via the VMMessageRequester and calls resetAccessControl().

So far so good, except this message has been sent with MuleClient.send() so its going to build a response using the request message's properties. This happens in MuleClient:330:

response = OptimizedRequestContext.unsafeRewriteEvent(response);

This eventually calls RequestContext.newMessage(message) which creates a copy of the message by doing new DefaultMuleMessage(payload, originalMessageAdapter) and resets the MessageAdapter that is also being picked up by the VMMessageRequester. Since it is not deterministic whether this happens after or before the VMMessageRequester gets the message and resets the access control, sometimes it fails and sometimes it doesn't.



 All   Comments   Work Log   Change History   Transitions   FishEye      Sort Order: Ascending order - Click to sort in descending order
Ross Mason added a comment - 05/May/08 11:36 AM
good synposis. maybe we should disable send() when using VM queues since you're not going to get a response from the queue anyway.

I did some further digging around the message scribbling and found the following -

1. Message rewriting has the biggest impact on performance (yourkit shows that way too much time is spent writing property maps)
2. we use RequestContext.safeRewriteEvent for writing response message properties when performing req/res messaging
3. We often use RequestContext.unsafeRewriteEvent as a safe-guard in case the message changed
4. When using send() we often return the original message when there is no result from the send operation.

All of this is throwback from Mule 1.x. We have fixed the route causes for doing these kinds of things in Mule 2.x except for one (request/response handling)

WRT Request Response handing (point 2), right now we 'merge' request and response messages which is confusing at best. The reason this is done is because you some times need properties on the request message to be available after you get a response back. I think there are two ways of handling this -

  • Have a specialisation of MuleMessage called RequestResponseMuleMessage that maintains a reference to both the request and the response message and provides interfaces for accessing the request message. This means people will have to check for the message type and cast it, or we make a bigger change to the send() method signature
  • Ignore the request message but provide a way let users configure which properties they want to be carried over from the request message to the response message. This could be done using a PropertyScope i.e. Session could be used, but not ideal. Or we could add a new scope for this purpose.

Point 3 should not be necessary any longer since Message payloads are not immutable as they were in Mule 1.x. The rewrite action is the most expensive. I believe we should only have to create a DefaultMuleMessage once for each request. This would improve performance.

Point 4. We would need to wrap a 'null' return into a NullPayload when using MuleClient.send() since we never return a null right now. This is partly be cause if anything does go wrong we can attach an exception payload to the return message. I think it would be less disruptive if we kept this behaviour.

IMO one of the biggest enabler of message scribbling is the VM transport because it allows the mule to "copy" the message over two threads. The other is our handling of request/response message flows and sync message flow in VM (which Dan points out).


Ross Mason added a comment - 05/May/08 11:37 AM
From Dan D.

> good synposis. maybe we should disable send() when using VM queues since you're not going to get a response from the queue anyway.

I think this makes a lot of sense...

> - Have a specialisation of MuleMessage called RequestResponseMuleMessage that maintains a reference to both the request and the response message and > provides interfaces for accessing the request message. This means people will have to check for the message type and cast it, or we make a bigger change to > the send() method signature

That seems kind of ugly to me at first glance. What about introducing the concept of an Exchange object which has references to the request/response messages. so you could do message.getExchange().getRequest() or getInMessage()


Ross Mason added a comment - 05/May/08 11:38 AM
From Andrew P.

>- Ignore the request message but provide a way let users configure which properties they want to be carried over from the request message to the response >message. This could be done using a PropertyScope i.e. Session could be used, but not ideal. Or we could add a new scope for this purpose.

New scope makes sense, which would be for the duration of the request. Quote often it's called 'flash' scope these days, when a single response still has access to the request and then the ref is automatically discarded (unlike e.g. session scope, which assumes the developer knowing when to drop the data).


Ross Mason added a comment - 05/May/08 11:42 AM
The other thing I think should be addressed is clearing the RequestContext once there is no request in progress. Right now there is an event associated with a thread even after the event has been processed as long as a new event is not available for the thread.
Once we are in the Messge Dispatcher (or returning to a callee in a MessageReceiver) we should clear the RequestContext. This would be cleaner and maintain the system in a consistent state.

Daniel Feist added a comment - 03/Jul/08 02:11 PM
Is there anything here that we should do before 2.0.2?

Ross Mason added a comment - 03/Sep/08 11:18 PM
It would be good to fix this, but I don't think its a priority for 2 EE right now. Keeping critical though

Daniel Feist added a comment - 04/Sep/08 05:21 PM
Did you mean to close this Ross or just reduce priority?

Ross Mason added a comment - 04/Sep/08 07:17 PM
oops, didn't mean to close this