Java HashMaps with only put and get - possibly concurrency issues?

Question

I've worked with ConcurrentHashMaps, but I'm not quite sure if that will cover all of the bases here.

I have a Spring Component. This Component will contain a Map. This will simply be a quick reference for objects in an external service. If the map does not contain a matching String, it will make a call to the external service, retrieve the object, and store it in the mapping. Then other classes can use the mapping for quick retrieval and usage. As such, there are only put() and get() operations performed on the map. Entries are never removed.

That being said, I'm a little concerned that ConcurrentHashMap may not provide the atomic control I'd like. Fetching SomeObject from the external service is potentially expensive. I'd rather not have two separate threads calling in at nearly the same time, resulting in multiple calls for the same value to the external service.

The idea is this:

Map<String, SomeObject> map = Collections.concurrentHashMap(
    new HashMap<String, SomeObject>());

public SomeObject getSomeObject(String key){
    if (!map.containsKey(key)){
        map.put(key, retrieveSomeObjectFromService(key));
    }
    return map.get(key);

Or this:

Map<String, SomeObject> map = new HashMap<String, SomeObject>();

public SomeObject getSomeObject(String key){
    synchronized(map){
        if (!map.containsKey(key)){
            map.put(key, retrieveSomeObjectFromService(key));
        }
    }
    return map.get(key);
}

The former is certainly simpler, but the latter would ensure that one two or more threads won't try to simultaneously trigger a fetching of the same SomeObject. Alternatively, I suppose I could try locking out only gets that attempt to retrieve a SomeObject that is already in the process of being fetched and does not block retrieving SomeObjects that already exist, but that would require a wait mechanism on the various string values and I'm not sure how to best implement that.

Answer 1

I would suggest you do a little bit of both!

Fast path, just 1 get out of the concurrent hashmap. Slow path, full sync and lock

private final ConcurrentHashMap<String, Object> map = new ConcurrentHashMap<String, Object>();
private final ReentrantLock lock = new ReentrantLock();

public Object getSomeObject(String key) {
    Object value = map.get(key);
    if (value == null) {
        try {
            lock.lock();
            value = map.get(key);
            if (value == null) {
                value = retrieveSomeObjectFromService(key);
                map.put(key, value);
            }

        } finally {
            lock.unlock();
        }

    }
    return value;
}

Do you understand why we need the 2nd get inside of the lock? Leaving that out leaves a case where we end up making the inside object twice, and having different copies of it floating around.

Also doing the assign the result to value and nullcheck vs using the contains method - understand why that is better? If we do a .contains then a .get, we just did 2 hashmap lookup. If I just do a get, I can cut my hashmap lookup time in half.

Another version Peter suggested.. less lines of code, but not my personal preference:

private final ConcurrentHashMap<String, Object> map = new ConcurrentHashMap<String, Object>();

public Object getSomeObject(String key) {
    Object value = map.get(key);
    if (value == null) {
        synchronized (map) {
            value = map.get(key);
            if (value == null) {
                value = retrieveSomeObjectFromService(key);
                map.put(key, value);
            }
        }
    }
    return value;
}