Is this Command Pattern Thread-safe?

Question

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;
    }
}

`HashMap` is not thread-safe. Immutable objects are, but you should use `final`. — SLaks, Oct 22 '13 at 18:28
Why not `public Command getCommand(String key) { return commandMap.get(key); }` ? (which will return null if the key is not in the map) — assylias, Oct 22 '13 at 18:30
@assylias That approach would be simpler and more performant, but I don't think it would make any difference to thread safety (especially when using HashMap). — Mike Clark, Oct 22 '13 at 18:39
The real question here is whether class loading/initializing creates a synchronization point, memory barrier, or memory flush point. If Thread A causes CommandController.class to be loaded and its static fields initialized, will thread B see up-to-date values for CommandController's static fields? Especially if there is no explicit synchronization point such as 'volatile', 'final', or 'synchronized'. I don't know. — Mike Clark, Oct 22 '13 at 18:47
@MikeClark It does. Writes that occur before the class is fully initialized are visible after a class is fully initialized. — John Vint, Oct 22 '13 at 18:49

John Vint · Answer 1 · 2013-10-22T18:54:00.473

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

The map is immutable (since it's read only).
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).

score 2 · Answer 2 · answered Oct 22 '13 at 18:44

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.

score 2 · Answer 3 · answered Oct 22 '13 at 19:01

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.

fommil · Answer 4 · 2013-10-22T19:32:36.747

1

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.

edited Oct 22 '13 at 19:32

answered Oct 22 '13 at 18:29

fommil

5,757
8
41
81

His `initMap` is valid code. It's using an [instance initializer](http://stackoverflow.com/questions/6763550/why-java-instance-initializers/). – Mike Clark Oct 22 '13 at 18:43

score 0 · Answer 5 · answered Oct 22 '13 at 18:38

0

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.

answered Oct 22 '13 at 18:38

R. Oosterholt

7,720
2
53
77

In the general case, simply ensuring that nothing is added or removed wouldn't be sufficient; you have to make sure that changes made by one thread are written to main memory, and that reading threads clear any cache and read from main memory at least once after initialization. – erickson Oct 22 '13 at 19:25

Is this Command Pattern Thread-safe?

5 Answers5