Should a method for lazily loading and caching an Object in a HashMap be synchronized?

Should this method be synchronized? I guess I don't understand how (and when) context switches can occur, so I don't know if it's possible for more than one thread to get into the if block inside my method.

public class ServiceLocator {
    private static final Map<Class, Object> applicationServices =
            new HashMap<Class, Object>();

    /**
     * Locates an application scoped service.  The service is lazy loaded and
     * will be cached permanently.
     *
     * @param type The type of service to locate.
     * @return An application scoped service of the speci开发者_StackOverflow社区fied type.
     */
    @SuppressWarnings({"unchecked"})
    public synchronized static <T> T getApplicationService(Class<T> type) {
        if(!applicationServices.containsKey(type)) {
            // If this method is NOT synchronized, is it possible for more than
            // one thread to get into this if block?
            T newService = ServiceLoader.create(type);
            applicationServices.put(type, newService);
            return newService;
        }
        return (T) applicationServices.get(type);
    }
}

---

Yes, you are absolutely right, this method requires synchronization. Technically context switch can occur between any two lines in your code, and even inside (e.g. between reading and storing i in ++i).

Note that using synchronized keyword does not mean that the thread won't be interrupted and the context switch will not occur while your thread is in synchronized block. It only means that any other thread that tries to execute the same code in other thread will block, effectively allowing the one owning the lock to continue.

Even using ConcurrentHashMap won't help you since safe putIfAbsent() method requires already existing value for a given key - and you probably don't want to eagerly create service each time.

However there are some better performing approaches to this quite common problem, e.g. see: How to implement Self Populating EHcache?

---

As others have mentioned you need synchronization to ensure that you don't create multiple instances of service.

You should look at the double checked locking idiom used for initializing singltons to improve performance here.

For example, getApplicationService itself does not need to be synchronized. In most cases it will find the service in cache and locking is only required when you don't find the element in the map:

public static <T> T getApplicationService(Class<T> type) {
    // applicationServices needs to be a ConcurrentHashMap otherwise 
    // the following containsKey can conflict with the subsequent put
    if(!applicationServices.containsKey(type)) {
        // need to synchronize before modifying the map
        synchronized(applicationServices) {
            // check if another thread created the same service while 
            // we were waiting for the lock
            if(!applicationServices.containsKey(type)) {
                T newService = ServiceLoader.create(type);
                applicationServices.put(type, newService);
            }
        }
    }
    return (T) applicationServices.get(type);
}

---

synchronized is the simpliest way to make it thread-safe, but not the most efficient one.

Without synchronized your code would have 2 problems:

• HashMap is not thread-safe, it should not be used from multiple threads without synchronization.
• One thread can enter an if block when another one (with the same type) is already inside it, thus two instances of the same type will be created.

So, if solution with synchronized doesn't satisfy you, you'll need to solve these two problems by other means. The first problem can be solved by using ConcurrentHashMap. The second one is more complex and can be solved by putting FutureTasks into map, something like this.

---

@Tomasz Nurkiewicz

It's possible to use putIfAbsent() to compete for a lock; the first thread gets to create the service; other threads wait to be notified. This way, we don't lock the whole map just for one service, which can get stuck. They even have a weird name for it, google "memoizer". Impls with FutureTask actually have subtle bugs or undesired behaviors in certain cases.

The need for such a utility - compute something on first demand then cache it - is quite strong. A good impl isn't trivial, and average programmers shouldn't bother to reinvent it. Java8 probably will ship something like it. For now people can use Guava's computing map.