How to perform concurrency safe mapped object operations?

Question

I read all and its contrary about Java ConcurrentHashMap usage. I hope my question will help clarify something that looks simple (with up to date answers).

I have a map like this:

ConcurrentHashMap<Integer, ClassA> map = new ConcurrentHashMap<Integer, ClassA>()

I am using a ConcurrentHashMap to keep put and get operation thread safe. My ClassA has some Integer/String attributes and a Collection of Strings.

Now I would like to know how to update my mapped object safely. If I want to create a method to update an object from my map by adding a new String, I have something like:

        synchronized(map)
        {
            Collection<String> strings = map.get(id).getStrings();
            if(!strings.contains(newString)) //strings can't be null
            {
                strings.add(newString);
            }
        }

Is this code safe against concurrent reads/write? Can it be done differently by using the Java API?

Your example has some mistakes probably. Right now the code inside synchronized block has nothing to do with map. There is a variable named events, but I don't know what is that - I guess it is your map. If that is the case, then you really don't need to synchronize on map. Extract this logic, push it into ClassA and then do some synchronization. — bbankowski, Jan 19 '15 at 22:22
Indeed, I updated the description with the correct name. It is indeed my map. I was hoping to synchronize the map to actually lock the entire table and prevent any concurrent changes on my mapped objects. Can you elaborate "some synchronization"? — TBag, Jan 19 '15 at 23:02
Simplest way would be to create public void synchronized add(String newString) method in your ClassA. Concurrent modifications of ClassA instances will be synchronized and you won't have to synchronize on map. — bbankowski, Jan 19 '15 at 23:12
@bbankowski If you make all operations safe by creating an external layer of synchronization, then it makes no sense to use ConcurrentHashMap — lbalazscs, Jan 20 '15 at 04:41
@lbalazscs I think there still might be, as we don't see put/get operations here. Of course, it would be good to see the whole class, to make a correct decision. — bbankowski, Jan 20 '15 at 08:27
Thanks all for trying to address my question. I posted a possible answer to it that is going in the same way as the use of a ConcurrentHashMap. I would appreciate your opinion! — TBag, Jan 20 '15 at 18:36

score 2 · Answer 1 · answered Jan 22 '15 at 23:18

What you have in your answer is not fully threadsafe. It replaces the value of the key id irrespective of what the old value was. If thats okay for your implementation then great but replace(K key, V oldObj, V newObj) is the ideal check and set (CAS) way to replace an existing value in ConcurrentMap.

In your particular use case, you would do the following

ClassA oldObj = map.get(id);
ClassA newObject = new ClassA(oldObject, newValue);
return map.replace(id, oldObj, newObject);

This ensures that the map is updated only if the previous value was oldObj. What happens if this code block is called by different threads with different newValue?. The above code would only let one thread succeed and the other thread would return false.

And if I want to prevent any thread race between the get and the replace, shall I synchronize on the map itself? — TBag, Jan 26 '15 at 16:02
You don't need to. At most 1 call to replace will succeed if there is a race. The 2nd time the replace will return false. If you want both to succeed you'd have to put it in a do/while loop — Nagesh Susarla, Jan 26 '15 at 23:16

score 0 · Answer 2 · edited May 23 '17 at 11:50

This is an attempt to answer my own question. It's highly inspired my another question answer I found: What is the preferred way to modify a value in ConcurrentHashMap?

If I make my ClassA immutable, and replace my code for updating my map objects that way:

        ClassA oldObject = map.get(id);
        ClassA newObject = new ClassA(oldObject, newValue);//this constructor copies the old one and add the newValue in my Collection of Strings
        map.put(id, newEvent);

Would it be thread safe? I really want to keep my code clean and efficient by using ConcurrentHashMap and I believe this solution is going in the same direction. I am just slightly suspicious about any thread retrieving the same object whyle another one would be between line 2 and 3.

How to perform concurrency safe mapped object operations?

2 Answers2