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