javaconcurrencythread-safetysynchronized

Thread Safety in operations involving ConcurrentHashMap in Java


Context: I'm creating a new Item, caching it and returning it, however, its name must be unique. We're talking about multithreaded env. My questions are in the comments.

class ItemOperations {

    private ConcurrentMap<String, Item> store = new ConcurrentHashMap<>();

    Item createItem(String name) throws Exception {

        // I agree this operation is thread safe because of ConcurrentHashMap
        if (store.containsKey(name)) {
            throw new Exception("Already Exists");
        }

        // Item creation is Expensive operation
        Item newItem = new Item();
        // *Ques 1* : For above - multiple threads will create multiple different objects right ?


        // putIfAbsent is again thread safe due to concurrentMap..
        // Losing thread will not update the value.. (correct me if i'm wrong)
        store.putIfAbsent("newItemName", newItem);


        // *Ques 2* : Separate threads will return different objects and
        // the second thread will return a new object which is 
        //NOT present in the store but was created on the fly , hence returning incorrect data
        return newItem;
    }
}

How to fix this ?

My thoughts: Should the whole createItem() be wrapped in a synchronized(this) block? so that Item Object creation is thread safe ? If yes, we'll lose the benefits of ConcurrentHashmap and it'll further degrade performance.

Using put instead of putIfAbsent is worse - ensures data correctness, but useless multiple writes to hashmap - hence impacting performance.


Solution

  • As written, your method is not threadsafe. Why?

    It is possible for two thread invocations of this method to reach this line and see a missing key:

    if (store.containsKey(name)) {
        throw new Exception("Already Exists");
     }
    

    If both invocations see the name key as missing, then your item now gets created twice. So what's the lesson here? Using ConcurrentHashMap on its own is not enough, at least not in the way you are using it.

    What you want to do is take advantage of ConcurrentHashMap's atomic methods. The answer posted by DuncG provides a way to do this:

    return map.computeIfAbsent(name, () -> new Item());
    

    Now, the ConcurrentHashMap takes care of the thread-safety for you, and you have no need of using the synchronized keyword. Effectively, it will check the map, create the object if it is missing, and return it, all atomically, without any concern for race conditions.

    All that said, if your use case can tolerate two or more creations of the Item() and two or more versions floating around (by chance), then it may not be that big of a deal. That said, I'd still recommend going with the computeIfAbsent approach since it guarantees consistent behavior.