A project I am working on has a lot of wrapper-classes for ConcurrentHashMap, and uses locks in an (I think) incorrect manner. Is this Correct?

Question

I have to work with a lot of classes that are used to store data in a ConcurrentHashMap and generally look like this:

import java.util.concurrent.ConcurrentHashMap;

import java.util.concurrent.locks.ReentrantLock;

public class Data {
    private ConcurrentHashMap<String, SomeObject> concurrentHashMap = new ConcurrentHashMap<String, SomeObject>();
    private ReentrantLock lock = new ReentrantLock();

    public void add(String string, SomeObject someObject) {
        try {
            lock.lock();
            concurrentHashMap.put(string, someObject);
        }
        finally {
            lock.unlock();
        }
    public void remove(String string) {
        try {
            lock.lock();
            concurrentHashMap.remove(string);
        }
        finally {
            lock.unlock();
        }
    public SomeObject get(String string) {
        try {
            lock.lock();
            return concurrentHashMap.get(string);
        }
        finally {
            lock.unlock();
        } 
    }

• is the lock even necessary? Because my intuition says it is not.

• this probably warrants a separate question, but if a ConcurrentHashMap contains values that are mutable, is it safe to modify them? let’s say that SomeObject has mutable fields and I do something like

    SomeObject someObject = data.get(someString);
    someObject.setMutableField(someValue);
    data.remove(someString);
    data.add(someString, so);
  

  would that be thread-safe? (already answered here What is the preferred way to modify a value in ConcurrentHashMap? and here Does re-putting an object into a ConcurrentHashMap cause a "happens-before" memory relation?)

• in some of those classes the ConcurrentHashMap is volatile - does it make any sense to make a ConcurrentHashMap volatile?

Answer 1

is the lock even necessary?

No, in your case (to add/remove/get a value) you don't need to use lock. ConcurrentHashMap is designed for such operation. But probably you may change your method "add" in the following manner:

    public SomeObject add(String string, PARAMETERS_TO_CONSTRUCT_SomeObject) {
        SomeObject result = concurrentHashMap.get(string);        
        if (result == null) {
            result = new SomeObject(PARAMETERS_TO_CONSTRUCT_SomeObject);
            SomeObject old = map.putIfAbsent(string, result);
            if (old != null) {
                result = old;
            }
        }
        return result;
    }

This guaranties you always have only one instance of SomeObject associated with the given key and prevents unnecessary instance creation/memory allocation.

does it make any sense to make a ConcurrentHashMap volatile?

I think the best way to publish your ConcurrentHashMap safely in this case is to define it as a final one:

    private final ConcurrentHashMap<String, SomeObject> concurrentHashMap = new ConcurrentHashMap<>();

Answer 2

is the lock even necessary? Because my intuition says it is not.

Do not rush to dismiss any purpose of the lock. The same lock may be used somewhere else, coordinating the updating of your shown map with some other operations mutually exclusive with it.

It may turn out that the usage of the ConcurrentHashMap instead of the plain HashMap was the overkill to eliminate, but even that is left open.

does it make any sense to make a ConcurrentHashMap volatile?

If the map variable may be mutated to point to a new map, it sure does make volatile necessary (providing there is no lock to protect that operation).

Answer 3

is the lock even necessary?

It is not necessary in the code that you have provided where the concurrent map and lock are both private variables of the class and the lock is only used to synchronize access to single method calls to the map. In this specific case, the use of the lock is removing any concurrency benefits provided by a concurrent hash map over a "normal" hash map.

in some of those classes the ConcurrentHashMap is volatile - does it make any sense to make a ConcurrentHashMap volatile?

If the variable pointing to the concurrent hash map is mutated to point to a different concurrent hash map instance, it would be important for the variable to either be volatile or for access to by synchronized using some other mechanism. In general, though, this seems like a pretty risky choice. Every use of the member variable will have to be closely examined to see that the behavior would be deterministic (and correct) in a threaded environment when the value changed. Take these 2 examples:

private volatile ConcurrentHashMap<K, V> map;

public int method1(K key, V value) {
  ConcurrentHashMap<K, V> localMap = map;
  localMap.putIfAbsent(key, value);
  return localMap.size();
}

public int method2(K key, V value) {
  map.putIfAbsent(key, value);
  return map.size();
}

public void mutatorMethod() {
  map = new ConcurrentHashMap<K,V>();
}

The 2 methods advertise to do the same thing: put a value for the key if one does not already exist and return the size of the map. The interesting case is that method2 could return 0 when called concurrently to mutatorMethod if the assignment to the new map occurred between the putIfAbsent and the size calls.