CODEWITHSUNDEEP
javamultithreadingdesign-patterns
Franz Muller
Franz Muller

Reputation: 21

Is this Command Pattern Thread-safe?

Hi Stackoverflow Community,

I have a Question about Thread-safety. If I has a static Map and fill it out with different Object, are these Object Thread-safe, if i has only method they don't write in it?

I create a small Example: Is the return value of getCommand thread safe in this case?

How can I test Thread-safety with JUnit?

Controller

public class CommandController {

    private static Map<String, Command> commandMap = initMap();


    public static Map<String, Command> initMap() {
         return new HashMap<String, Command>() {{
             put("A", new CommandA());
             put("B", new CommandB());
         }};
    }


    public Command getCommand(String key) {
        if(commandMap.containsKey(key)) {
            return commandMap.get(key);
        }
        return null;
    }

}

Abstract Class

public abstract class Command {

    public abstract int calc(int value);

}

Command A

public class CommandB extends Command {
    @Override
    public int calc(int value) {
        value = value * 4;
        return value; 
    }
}

Command B

public class CommandA extends Command {
    private int ab = 5;

    @Override
    public int calc(int value) {
        return value * ab;
    }
}

Upvotes: 2

Views: 903

Answers (5)

fommil
fommil

Reputation: 5885

This is thread safe because nobody can access your your Map, and so cannot mutate it. However, you might want to make it private static final just to be sure that there are no memory visibility problems.

I do this sort of thing all the time (but not with static maps) - I use Spring to populate the Map.

Upvotes: 1

meriton
meriton

Reputation: 70574

Yes. The Java Language Specification writes:

If a program has no data races, then all executions of the program will appear to be sequentially consistent.

where a data race occurs if two conflicting accesses to the same variable are not in a happens-before relationship, and

Two accesses to (reads of or writes to) the same variable are said to be conflicting if at least one of the accesses is a write.

Concurrent access to a map only reads shared state, and reads can only conflict with writes. Therefore, it is sufficient to prove that the initialization of the map happens-before it is being accessed by the concurrent threads.

This is indeed the case, because initialization occurs in a static field initializer, which are processed during class initialization. The spec requires that a class is initialized before a method it declares is invoked, and the detailed initialization procedure employs synchronization to ensure that initialization occurs only once, and the initializing thread synchronizes-with all other threads that access the class, thereby establishing happens-before.

As a matter of style, you might wish to declare the field final to emphasize that it is only assigned during class load time, and access to the field requires no further sychronization.

Upvotes: 2

John Vint
John Vint

Reputation: 40256

This is thread-safe for two reasons. In this case, both need to be accounted for in order to have pure thread-safety

  1. The map is immutable (since it's read only).
  2. It's initialized with the class. Since class initialization is thread-safe and guarantees publication, visibility is not an issue.

Note: Slaks does bring up a good point. You should use final. Usually if you are worried about thread-safety and the field is neither final nor volatile, there is probably something wrong. Though making it final in this case does not make it more thread safe it just prevents something thread-unsafe happening in the future (like re assigning it).

Upvotes: 4

erickson
erickson

Reputation: 269797

Yes, this is thread safe, because class initialization is guaranteed to be visible to all threads that use the class, and your map is "effectively immutable"—its state doesn't change after class initialization.

However, if you initialized the map from some static method that your program invoked explicitly during a setup phase, you'd have to implement your own memory barrier to make sure other threads could see the correct state of the map. So, make sure the map is fully initialized during class initialization; that's what makes this work.

Upvotes: 2

R. Oosterholt
R. Oosterholt

Reputation: 8090

Seems thread safe to me. Nothing is added/removed from the map in CommandController. And the Commands (CommandA and CommandB) do not have private variable which are modified (they are only used in the calculation.

This is a simple example of a (much) more complex situation I guess, so when your real-life situation manipulates the map in CommandController or when the Command do have class variables which are modified, they you would have a concurrency issue.

Upvotes: 0

Related Questions