stateful dependency and thread safety

Question

I've Struts 1 action class (actions are singletons by design in struts 1) that needs to collect some data and then combine them all into single response. I'd like to extract all the response generation logic into separate class called

ResponseBuilder

Normally I'd put ResponseBuilder as a field and have setter for it (e.g. for testing). My response builder looks as follows

class JsonResponseBuilder implements ResponseBuilder {
    public void addElement(String key, Object value) {
        ...
    }

    public String buildResponse() {
        // build response from data collected
    }
}

With such implementation I can't do it due to thread safety issues here.

How can I change this design to be ok? Is Factory pattern here applicable? I mean is using

ResponseBuilderFactory

as a dependency and calling it that way:

ResponseBuilder builder = factory.getBuilder();
builder.addElement(...);
...
String response = builder.build();

is ok from design and testability point of view? If it is ok. how to write test code for this? Mock factory? Mock builder?

score 2 · Accepted Answer · edited May 23 '17 at 12:27

A factory would work. You're essentially doing a form of Dependency Injection with a factory. When you instantiate or initialise your action, you would set the factory:

public void setResponseFactory(ResponseBuilderFactory factory) {
    this.responseFactory = factory;
}

and in the factory, just return a new instance of JsonResponseBuilder when you need it. Make sure that you do not store your instance of JsonResponseBuilder as an instance variable in your action; it must remain local to the method you're using, or passed around as a method parameter.

As for testing, it becomes easy to replace the factory with a mock factory that returns a mock ResponseBuilder. There are many libraries to do this, like Mockito or JMock. All of them work well with JUnit and TestNG.

Edit:

You'd need to have ResponseBuilderFactory as an interface, like:

public interface ResponseBuilderFactory {
    public ResponseBuilder getResponseBuilder();
}

When you do your testing, just create a class that returns a mock of your ResponseBuilder:

@Test
public void testMyAction() throws Exception {
    ResponseBuilderFactory mockFactory = new ResponseBuilderFactory() {
        public ResponseBuilder getResponseBuilder() {
            ResponseBuilder builder = context.mock(ResponseBuilder.class);
            // set up mock behaviour
            return builder;
        }
    }
}

So you're not injecting a mock factory, just a factory that returns mocks.

Also, see Dependency Injection vs Factory Pattern.

Edit 2:

If you can somehow manage to re-code your JsonResponseBuilder so that it doesn't maintain state, then you could potentially avoid the whole factory mess all together and just use your original approach. Objects that do not maintain state are inherently thread-safe.

Just a dumb question, isn't mock (factory) returning another mock (builder) a weird thing? Seems logical here but I don't know yet (newbie here) if it is ther right way. — grafthez, Dec 16 '11 at 13:45
You could make a normal factory that returns a mock :). I'll update my answer with details. — Jonathan, Dec 16 '11 at 13:53
As said in another answer, this seems like a case of changing design solely for the purposes of testing. May make sense, but don't get too far. Imho, if there is no interface for "ResponseBuilder" or "ResponseBuilderFactory", then this should just be used as "new ResponseBuilder()" and the problem is related to testing, not to design. Note that *there are* testing solutions that work for the "new ResponseBuilder()" case (shall that be needed), or you should be using a DI container. — jjmontes, Dec 16 '11 at 14:08

score 2 · Answer 2 · answered Dec 16 '11 at 13:36

2

Just instantiate your ResponseBuilder within your action service method. That way it will be a local variable (instead of a singleton class member) and each thread will have a distinct fresh instance of ResponseBuilder.

You can unit test the ResponseBuilder separately, and you can also test your action normally as with any other action (mocking the ResponseBuilder if needed).

Using a factory pattern to build your ResponseBuilder is a different matter, not related to your issue, imo. If instantiating ResponseBuilders is expensive and they are reusable, then you may wish to use a pool of ResponseBuilders.

answered Dec 16 '11 at 13:36

jjmontes

24,679
4
39
51

But having "new JsonResponseBuilder()" in action's method stops me from mocking this builder, right? Or am I missing something important here? – grafthez Dec 16 '11 at 13:41
Well that's a testing aspect... and depends on what library you use to test. Powermock can mock constructors, although, like you, I normally prefer to avoid it and use injected dependencies. But I hate changing code solely for the purposes of testing. If you don't want to consider the builder as part of the action and you don't want to mock constructors (i.e. you want to use Mockito but not Powermock), your approach is perfectly fine. But then, I'd strongly suggest you to use Spring (or any other DI container), which can be configured to provide singletons or new instances at your will. – jjmontes Dec 16 '11 at 13:58
...and: Mocking constructors with Powermock: http://code.google.com/p/powermock/wiki/MockConstructor (I try to avoid this, but again, sometimes you just need to mock constructors or static methods, and Powermock is a solution). – jjmontes Dec 16 '11 at 14:00

score 1 · Answer 3 · answered Dec 16 '11 at 13:37

1

The minute you make your ResponseBuilder a member variable in an Action, it's shared across all requests. That means you'll have to synchronize it to ensure that its mutable state, if any, is thread safe.

Another way to do it is to create a new ResponseBuilder inside the method called for each action. You create an instance for each request that way.

answered Dec 16 '11 at 13:37

duffymo

305,152
44
369
561

I'd be hard to synchronize such thing if it's responsibility is to collect various data during one request and prepare response from this data. I suppose I should block all other threads from executing this action to have it safe. – grafthez Dec 16 '11 at 13:44
Exactly - that's why you don't want it as a data member. – duffymo Dec 16 '11 at 14:47

score 1 · Answer 4 · answered Dec 16 '11 at 13:39

You could store the Response builder in the session. Make it appear as a 'property' of your Action class by having getter/setter for it, but don't define an actual field. Instead do something like:

protected ResponseBuilder getResponseBuilder() {
    ResponseBuilder builder = (ResponseBuilder) session.getAttribute("ATTR_RESPONSE_BUILDER");
    if(builder == null) {
        builder = new ResponseBuilder();
        session.setAttribute("ATTR_RESPONSE_BUILDER", builder);
    }

    return builder;
}

protected void setResponseBuilder(ResponseBuilder builder) {
    session.setAttribute("ATTR_RESPONSE_BUILDER", builder);
}

You'll need to clear the builder in between calls to the Action class.

stateful dependency and thread safety

4 Answers4