State vs. Method Parameters Lesson: IllegalStateExcpetions in a Mule Transformer

Proper use of state versus passing values around as parameters seems like a topic one should have down pretty well by some point in their programming careers, and therefore would not be the focus of too much attention as time goes on. I, however, have run into a situation recently where not paying enough attention to it has caused me some problems, which in hindsight make me feel like I was asking for them. To summarize, storing a variable as a property in class when I had no real reason to, even when it felt like it could be justified, proved to be troublesome.

The context of the problem involved my implementation of a Mule transformer, in which I had to implement a doTransform() method to transform a response from my service if the incoming service request had a specific property. The MuleMessage containing the property I'm interested in can be retrieved statically via the RequestContext and can be thought of to live through the lifetime of the request (i.e. until a response is sent).

Here's the code conceptually:

import org.mule.RequestContext
import org.mule.transformer.AbstractTransformer

class ResponseTransformer extends AbstractTransformer {

    def muleMessage

    @Override
    protected Object doTransform(Object response, String encoding) {
        try {
            muleMessage = retrieveMuleMessage()
            if (isSpecialResponseExpected()) {
                def attributes = retrieveAttributesFromMessage()
                response = makeMessageSpecial(response, attributes) 
            }
        } catch(e) {
            throw new TransformerException(this, e)
        }        
        return response
    }

    def retrieveMuleMessage() { RequestContext.event.message }
    boolean isSpecialResponseExpected() { muleMessage.getProperty("special") }
    def retrieveAttributesFromMessage() { // do something with muleMessage property.. }
    def makeResponseSpecial(response, attributes) { // transform response here ... }

}

There were no problems with the code as written when I was sending only one message at a time to my service. When the service had an opportunity to grab and process more than one message concurrently, however, I would consistently run into the following excpetion:

Caused by: java.lang.IllegalStateException: Only owner thread can write to message: Thread[asyncDelivery4,5,main]/Thread[asyncDelivery2,5,main]
        at org.mule.transport.AbstractMessageAdapter.newException(AbstractMessageAdapter.java:619)
        at org.mule.transport.AbstractMessageAdapter.checkMutable(AbstractMessageAdapter.java:605)
        at org.mule.transport.AbstractMessageAdapter.assertAccess(AbstractMessageAdapter.java:546)
        at org.mule.transport.AbstractMessageAdapter.setProperty(AbstractMessageAdapter.java:244)

Obviously Mule must not be very thread safe, right? I'm obviously not doing anything bad in my code to deserve this...

It's always easier to make something you had no hand in writing the first target for all your problems, but the truth of the matter is, by saving the retrieved Mule message in the muleMessage property, I am giving my transformer state.

Thinking functionally, the same request object and message properties should always map to or produce the same result. Therefore, the transformer could be implemented with one big method where all my code would reside, the doTransform() method here. On the other hand, with one conceptual way in and one way out, it couldn't hurt to just store the Mule message in a property instead of passing it around, right?

It seems however that transformers are instantiated in Mule as singletons (don't quote me on that), or at the very least I was not doing anything to ensure or verify they weren't. So by storing the message as a property, concurrent message threads were trying to access the same Mule message. The lesson here, (for me at least), is that I probably shouldn't store variables as properties unless I have a good reason to, when having state makes sense.

The revised code:

class ResponseTransformer extends AbstractTransformer {

    @Override
    protected Object doTransform(Object response, String encoding) {
        try {
            def muleMessage = retrieveMuleMessage()
            if (isSpecialResponseExpected(muleMessage)) {
                def attributes = retrieveAttributesFromMessage(muleMessage)
                response = makeMessageSpecial(response, attributes) 
            }
        } catch(e) {
            throw new TransformerException(this, e)
        }        
        return response
    }

    def retrieveMuleMessage() { RequestContext.event.message }

    boolean isSpecialResponseExpected(muleMessage) { muleMessage.getProperty("special") }
    def retrieveAttributesFromMessage(muleMessage) { // do something with muleMessage property.. }
    def makeResponseSpecial(response, attributes) { // transform response here ... }
}

Passing the Mule message around is more functional, and could at the very least help me with my unit tests, where I wouldn't have to set the "state" of the transformer to test the methods that could have just as easily have another parameter in their signatures.