Do I need client side locking while checking if a key exist or not in ConcurrentHashMap?

Question

I know I could use the putIfAbsent of ConcurrentHashMap. However, I need to do a webservice call to fetch a value for the given key if it does not exist and then store it (kind of caching) so I don't need to the next time same key is being used. Which of the following is correct? I think second version is necessary with the synchronized.

Update 1: I cannot use any functional interface.

Update 2: Updating the code snippet based on the reply from Costi Ciudatu

private static final Map<String, String> KEY_VALUE_MAP = new ConcurrentHashMap<>();
public String getValueVersion1(String key) {
    String value  = KEY_VALUE_MAP.get(key);
    if (value != null) {
        return value;
    }

    // Else fetch the value for the key from the webservice.
    value = doRestCall(key);
    KEY_VALUE_MAP.putIfAbsent(key, value);

    return value;
} // Version 1 Ends here.

public synchronized String getValueVersion2(String key) {
    String value  = KEY_VALUE_MAP.get(key);
    if (value != null) {
        return value;
    }

    // Else fetch the value for the key from the webservice.
    value = doRestCall(key);
    KEY_VALUE_MAP.put(key, value);
    return value;
} // Version 2 ends here.

Answer 1

You should have a look at ConcurrentMap#computeIfAbsent which does that for you atomically:

return KEY_VALUE_MAP.computeIfAbsent(key, this::doRestCall);

---

Edit (to address your "no functional interface" constraints):

You only need "client side locking" if you want to make sure that you only invoke doRestCall once for any given key. Otherwise, this code would work just fine:

final String value = KEY_VALUE_MAP.get(key);
if (value == null) {
    // multiple threads may call this in parallel
    final String candidate = doRestCall(key);
    // but only the first result will end up in the map
    final String winner = KEY_VALUE_MAP.putIfAbsent(key, candidate);
    // local computation result gets lost if another thread made it there first
    // otherwise, our "candidate" is the "winner"
    return winner != null ? winner : candidate;
}
return value;

However, if you do want to enforce that doRestCall is invoked only once for any given key (my guess is you don't really need this), you will need some sort of synchronization. But try to be a bit more creative than the "all-or-nothing" approach in your examples:

final String value = KEY_VALUE_MAP.get(key);
if (value != null) {
    return value;
}
synchronized(KEY_VALUE_MAP) {
    final String existing = KEY_VALUE_MAP.get(key);
    if (existing != null) {  // double-check
        return existing;
    }
    final String result = doRestCall(key);
    KEY_VALUE_MAP.put(key, result);  // no need for putIfAbsent
    return result;
}

If you want to use this second (paranoid) approach, you can also consider using the key itself for locking (to narrow the scope to the minimum). But this would probably require you to manage your own pool of keys, as syncrhonized (key.intern()) is not good practice.

This all relies on the fact that your doRestCall() method never returns null. Otherwise, you'll have to wrap the map values within an Optional (or some home-made pre-java8 alternative).

As a (final) side note, in your code samples you inverted the use of put() and putIfAbsent() (the latter is the one to use with no external synchronization) and you read the value twice for null-checking.