Is the following code thread-safe?

Question

The following code uses a double checked pattern to initialize variables. I believe the code is thread safe, as the map wont partially assigned even if two threads are getting into getMap() method at the same time. So I don't have to make the map as volatile as well. Is the reasoning correct? NOTE: The map is immutable once it is initialized.

class A {

    private Map<String, Integer> map;
    private final Object lock = new Object();

    public static Map<String, Integer> prepareMap() {
        Map<String, Integer> map = new HashMap<>();
        map.put("test", 1);
        return map;
    }

    public Map<String, Integer> getMap() {
        if (map == null) {
            synchronized (lock) {
                if (map == null) {
                    map = prepareMap();
                }
            }
        }
        return map;
    }
}

Answer 1

According to the top names in the Java world, no it is not thread safe. You can read why here: http://www.cs.umd.edu/~pugh/java/memoryModel/DoubleCheckedLocking.html

You better off using ConcurrentHashmap or synchronizing your Map.

http://docs.oracle.com/javase/7/docs/api/java/util/concurrent/ConcurrentHashMap.html

Edit: If you only want to make the initialization of the map thread safe (so that two or more maps are not accidentally created) then you can do two things. 1) initialize the map when it is declared. 2) make the getMap() method synchronized.

Answer 2

No, your reasoning is wrong, access to the map is not thread safe, because the threads that call getMap() after the initialization may not invoke synchronized(lock) and thus are not in happens-before relation to other threads.

The map has to be volatile.

Answer 3

The code could be optimized by inlining to

  public Map<String,Integer> getMap()
  {
     if(map == null)
     {
        synchronized(lock)
        {
           if(map == null)
           {
               map = new HashMap<>();  // partial map exposed
               map.put("test", 1);               
           }
        }
     }
     return map;
  }
}

Having a HashMap under concurrent read and write is VERY dangerous, don't do it.
Google HashMap infinite loop.

---

Solutions -

Expand synchronized to the entire method, so that reading map variable is also under lock. This is a little expensive.

Declare map as volatile, to prevent reordering optimization. This is simple, and pretty cheap.

Use an immutable map. The final fields will also prevent exposing partial object state. In your particular example, we can use Collections.singletonMap. But for maps with more entries, I'm not sure JDK has a public implementation.

Answer 4

This is just one example of how things can go wrong. To fully understand the issues, there is no substitute for reading The "Double-Checked Locking is Broken" Declaration, referenced in a prior answer.

To get anything approaching the full flavor, think about two processors, A and B, each with its own caches, and a main memory that they share.

Suppose Thread A, running on Processor A, first calls getMap. It does several assignments inside the synchronized block. Suppose the assignment to map gets written to main memory first, before Thread A reaches the end of the synchronized block.

Meanwhile, on Processor B, Thread B also calls getMap, and does not happen to have the memory location representing map in its cache. It goes out to main memory to get it, and its read happens to hit just after Thread A's assignment to map, so it sees a non-null map. Thread B does not enter the synchronized block.

At this point, Thread B can go ahead and attempt to use the HashMap, despite the fact that Thread A's work on creating it has not yet been written to main memory. Thread B may even have the memory pointed to by map in its cache because of a prior use.

If you are tempted to try to work around this, consider the following quote from the referenced article:

There are lots of reasons it doesn't work. The first couple of reasons we'll describe are more obvious. After understanding those, you may be tempted to try to devise a way to "fix" the double-checked locking idiom. Your fixes will not work: there are more subtle reasons why your fix won't work. Understand those reasons, come up with a better fix, and it still won't work, because there are even more subtle reasons.

This answer only contains one of the most obvious reasons.

Answer 5

No, it is not thread safe.

The basic reason is that you can have reordering of operations you don't even see in the Java code. Let's imagine a similar pattern with an even simpler class:

class Simple {
    int value = 42;
}

In the analogous getSimple() method, you assign /* non-volatile */ simple = new Simple (). What happens here?

1. the JVM allocates some space for the new object
2. the JVM sets some bit of this space to 42 (for value)
3. the JVM returns the address of this space, which is then assigned to space

Without synchronization instructions to prohibit it, these instructions can be reordered. In particular, steps 2 and 3 can be ordered such that simple gets the new object's address before the constructor finishes! If another thread then reads simple.value, it'll see a value 0 (the field's default value) instead of 42. This is called seeing a partially-constructed object. Yes, that's weird; yes, I've seen things like that happen. It's a real bug.

You can imagine how if the object is a non-trivial object, like HashMap, the problem is even worse; there are a lot more operations, and so more possibilities for weird ordering.

Marking the field as volatile is a way of telling the JVM, "any thread that reads a value from this field must also read all operations that happened before that value was written." That prohibits those weird reorderings, which guarantees you'll see the fully-constructed object.

Answer 6

Unless you declare the lock as volatile, this code may be translated to non-thread-safe bytecode.

The compiler may optimize the expression map == null, cache the value of the expression and thus read the map property only once.

volatile Map<> map instructs the Java VM to always read the property map when it is accessed. Thsi would forbid such optimization from the complier.

Please refer to JLS Chapter 17. Threads and Locks