Java and thread safety when creating a cache from scratch

Question

Is the following code thread safe?

private static HashMap<String, String> cache = null;

public static String getInfo(String key) {
    populateCache();

    return cache.containsKey(key) ? cache.get(key) : null;
}

private static void populateCache() {
    if (cache == null) {
        cache.put("hello", "world");

        // ... more population logic here
    }
}

If not, what can I do to make it thread safe?

EDIT: Thanks for the answers, I'll try out ConcurrentHashMap. However, I missed out something.. I am actually passing an object of a separate Foo class to getInfo() here:

public static Bar getInfo(Foo object) {
    object.someProperty = concurrentHashMapCache.get('something');

    return Bar.createFromFoo(object);
}

As long as different threads pass a different Foo object to getInfo, the above code should work, right?

Answer 1

HashMaps in Java are not thread safe. If you want a thread safe version use ConcurrentHashMap: http://docs.oracle.com/javase/7/docs/api/java/util/concurrent/ConcurrentHashMap.html

Answer 2

It's not. Either use ConcurrentHashMap or make the public method synchronized (which makes more sense since you've got a race condition also on creating the HashMap).

Answer 3

Much depends not only on simply how a class is written, but also on how it will be used.

You have a single shared mutable field which makes up the state of your class's objects.

cache is populated without any attempts to synchronize access, so it is possible in a concurrent environment for two client threads to be modifying cache at the same time.

In the absence of any kind of internal syncronization, this class looks unsafe.