Lazily initialize a Java map in a thread safe manner

Double Check Locking

Double check locking requires several steps all to be completed in order to work properly, you are missing two of them.

First you will need to make someMap into a volatile variable. This is so that other threads will see changes made to it when they are made but after the changes are complete.

private volatile Map<String, String> someMap = null;

You also need a second check for null inside the synchronized block to make sure that another thread hasn't initialized it for you while you were waiting to enter the synchronized area.

    if (someMap == null) {        synchronized(this) {            if (someMap == null) {

Do not assign until ready for use

In your generation of the map construct it in a temp variable then assign it at the end.

                Map<String, String> tmpMap = new HashMap<String, String>();                // initialize the map contents by loading some data from the database.                // possible for the map to be empty after this.                someMap = tmpMap;            }        }    }    return someMap.get(key); 

To explain why the temporary map is required. As soon as you complete the line someMap = new HashMap... then someMap is no longer null. That means other calls to get will see it and never try to enter the synchronized block. They will then try to get from the map without waiting for the database calls to complete.

By making sure the assignment to someMap is the last step within the synchronized block that prevents this from happening.

unmodifiableMap

As discussed in the comments, for safety it would also be best to save the results in an unmodifiableMap as future modifications would not be thread safe. This is not strictly required for a private variable that is never exposed, but it's still safer for the future as it stops people coming in later and changing the code without realizing.

            someMap = Collections.unmodifiableMap(tmpMap);

Why not use ConcurrentMap?

ConcurrentMap makes individual actions (i.e. putIfAbsent) thread-safe but it does not meet the fundamental requirement here of waiting until the map is fully populated with data before allowing reads from it.

Additionally in this case the Map after the lazy initialization is not being modified again. The ConcurrentMap would add synchronization overhead to operations that in this specific use case do not need to be synchronized.

Why synchronize on this?

There is no reason. :) It was just the simplest way to present a valid answer to this question.

It would certainly be better practice to synchronize on a private internal object. You have improved encapsulation traded off for marginally increased memory usage and object creation times. The main risk with synchronizing on this is that it allows other programmers to access your lock object and potentially try synchronizing on it themselves. This then causes un-needed contention between their updates and yours, so an internal lock object is safer.

In reality though a separate lock object is overkill in many cases. It's a judgement call based on the complexity of your class and how widely is is used against the simplicity of just locking on this. If in doubt you should probably use an internal lock object and take the safest route.

In the class:

private final Object lock = new Object();

In the method:

synchronized(lock) {

As for java.util.concurrent.locks objects, they don't add anything useful in this case (although in other cases they are very useful). We always want to wait until the data is available so the standard synchronized block gives us exactly the behavior we need.

I think TimB explained most of the options very well, but I think the quickest and most obvious answer would be to create it when the class instance is instantiated.

class SomeClass {    private final Map<String, String> someMap = new HashMap<String, String>();    public String getValue(String key) {        return someMap.get(key);  // the key might not exist even after initialization    }}

The reason you want to lazy initialize your map is because the generation of values is resource intensive. Generally you can differentiate between two usecases

1. Generation/storage of each value is equally expensive
2. Generation of values is expensive but if you generate one, generating the rest is not that expensive anymore (for example you need to query a database)

The Guava library has a solution for both. Use a Cache to generate values on the fly or a CacheLoader + loadAll to bulk generate values. Since the initialization of an empty Cache is virtually free there is no need to use the double check idiom: simply assign the Cache instance to a private final field.