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

Question

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

score 5 · Answer 1 · edited May 23 '17 at 10:34

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?

Miserable Variable · Accepted Answer · 2011-10-18T10:56:32.127

2

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

edited Oct 18 '11 at 10:56

answered Oct 17 '11 at 08:46

Miserable Variable

28,432
15
72
133

All of the answers look correct and I appreciate them. I've selected this as the correct answer because it is very simple, yet accomplishes what I want. – Ryan J Oct 17 '11 at 22:10
2

Note that `applicationServices` should be a `ConcurrentHashMap`. Otherwise calling first `containsKey` and `put` might occur concurrently, which is not safe with `HashMap` (?) – Tomasz Nurkiewicz Oct 18 '11 at 06:25
@TomaszNurkiewicz thanks, yes that is correct. I will amend my answer. Even with that restriction, do you think this will be better than making the entire method synchronized? – Miserable Variable Oct 18 '11 at 07:12
There is no easy way to tell. It depends on the amount of concurrency and cache misses. Unfortunately the only valid result is: you have to benchmark both approaches. – Tomasz Nurkiewicz Oct 18 '11 at 08:21
I think [this](http://www.ibm.com/developerworks/java/library/j-dcl/index.html) older, but good, article has some useful info on double checked locking. – Ryan J May 08 '12 at 18:09
1

@RyanJ thanks for the pointer. I was under the impression the double-checked-locking works fine 1.5 onwards but the editor's note on that article says it is broken. Also see http://stackoverflow.com/a/157213/18573 – Miserable Variable May 08 '12 at 20:03

score 1 · Answer 3 · edited May 23 '17 at 12:34

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.

score 0 · Answer 4 · answered Oct 18 '11 at 15:53

@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.

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

4 Answers4