When and how should I use additional synchronization of ConcurrentHashMap?

Question

I need to know when I should add some synchronization block to my code when using ConcurrentHashMap. Let's say I have a method like:

private static final ConcurrentMap<String, MyObjectWrapper> myObjectsCache = new ConcurrentHashMap<>(CACHE_INITIAL_CAPACITY);

    public List<MyObject> aMethod(List<String> ids, boolean b) {
        List<MyObject> result = new ArrayList<>(ids.size());
        for (String id : ids) {
            if (id == null) {
                continue;
            }
            MyObjectWrapper myObjectWrapper = myObjectsCache.get(id);
            if (myObjectWrapper == null) {
                continue;
            }
            if (myObjectWrapper.getObject() instanceof MyObjectSub) {
                ((MyObjectSub) myObjectWrapper.getObject()).clearAField();
                myObjectWrapper.getObject().setTime(System.currentTimeMillis());
            }
            result.add(myObjectWrapper.getObject());
            if (b) {
                final MyObject obj = new MyObject(myObjectWrapper.getObject());
                addObjectToDb(obj);
            }
        }
        return result;
    }

How should I efficiently make this method concurrent? I think that the "get" is safe but once I get the value from cache and update the cached object's fields - there can be problems beacuse another thread could get the same wrapper and try to update the same underlying object... Should I add synchronization? And if so, then should I synchronize from "get" to end of loop iteration or the entire loop?

Maybe someone could share some more specific guidelines of proper and efficient use of ConcurrentHashMap when some more operations need to be done on the map keys/values inside loops etc...

I would be really grateful.

EDIT: Some context for the question: I'm currently working on refactoring of some dao classes in production code and a few of the classes used HashMaps for caching data retrieved from the database. All methods that used the cache (for write or reads) had their entire content inside a synchronized(cache) block (playing safe?). I don't have much experience with concurrency and I really want to use this opportunity to learn. I naively changed the HashMaps to ConcurrentHashMaps and now want to remove the synchronized bloocks where they're necessary. All caches are used for writes and reads. The presented method is based on one of the methods that I've changed and now I'm trying to learn when and to what extent synchronize. The methods clearAField just changes a value of one of the fields of the wrapped POJO object and addObjectToDb tries to add the object to the database.

Other example would be refilling of the cache:

public void findAll() throws SQLException{
    // get data from database into a list
    List<Data> data=getAllDataFromDatabase();
    cacheCHM.clear();
    cacheCHM.putAll(data);
} 

In which case I should put the clear and putAll inside a synchronize(cacheCHM) block, right?

I've tried to find and read some posts/articles about the proper and efficient usage of CHM but most deal with single operations, without loops etc.... The best I've found would be: http://www.javamadesoeasy.com/2015/04/concurrenthashmap-in-java.html

Answer 1

A better approach to threadsaftey and caches is to use immutable objects. Here's what the MyObjectSub calss might look like if it were immutable [not sure why you need the wrapper - I'd omit that completely is possible]:

//Provided by way of example. You should consider generating these 
//using http://immutables.github.io/ or similar
public class MyImmutableObject {
    //If all members are final primitives or immutable objects 
    //then this class is threadsafe.
    final String field;
    final long time;

    public MyImmutableObject(String field, long time) {
        this.field = field;
        this.time = time;
    }

    public MyImmutableObject clearField() {
        //Since final fields can never be changed, our only option is to 
        //return a copy.
        return new MyImmutableObject("", this.time);
    }

    public MyImmutableObject setTime(long newtime) {
        return new MyImmutableObject(this.field, newtime);
    }
}

If your objects are immutable then thread safety is a lot simpler. Your method would look something like this:

public List<Result> typicialCacheUsage(String key) {
    MyImmutableObject obj = myObjectsCache.get(key);

    obj = obj.clearField();
    obj = obj.setTime(System.currentTimeMillis());

    //If you need to put the object back in the cache you can do this:
    myObjectsCache.put(key, obj);

    List<Result> res = generateResultFromObject(obj);
    return res;
}

Answer 2

You've not mentioned what concurrency you expect to happen within your app, so I'm going to assume you have multiple threads calling aMethod, and nothing else.

You only have a single call to the ConcurrentHashMap: myObjectsCache.get(id), this is fine. In fact since nothing is writing data into your objectCache [see assumption above] you don't even need a ConcurrentHashMap! You'd be fine with any immutable collection. You have a suspicious line at the end: addObjectToDb(obj), does this method also affect your cache? If so it's still safe (probably, we'd have to see the method to be certain), but you definitely need the ConcurentHashMap.

The danger is where you change the objects, here:

myObjectWrapper.getObject().clearAField();
myObjectWrapper.getObject().setTime(System.currentTimeMillis());

It's possible for multiple threads to call these methods on the same object at the same time. Without knowing what these methods do, we can't say if this is safe or not. If these methods are both marked synchronised, or if you took care to ensure that it was safe for these methods to run concurrently then you're fine (but beware there's scope for these methods to run in different orders to what you might intuitively expect!). If you weren't so careful then there is a potential for data corruption.