concurrent hashmap and copyonwritearraylist

Question

I am trying to populate a cache which hold the key/value with ConcurrentHashMap.

I am assuming using a CopyOnWriteArrayList takes care of concurrency and I have that as my value for my key, but I am missing something in the below code and it is overriding its values when multiple threads are executing.

if (testMap.get(id) == null) {
   CopyOnWriteArrayList<String> copyArr = new CopyOnWriteArrayList<String>();
   copyArr.add("Add Value");
   testMap().putIfAbsent(id, copyArr);
} else {                    
   testMap.put(id,testMap.get().add("Append Value"));
}

How do I protect the code which creates the CopyOnWriteArrayList from multiple threads.

Here is the revised version of code as per the suggestions below.

CopyOnWriteArrayList<Subscriber> subscriberArr =  CacheUtils.getSubscriberMap().get(syncDet.getCardNumber());

if (subscriberArr == null) {

subscriberArr = new CopyOnWriteArrayList<Subscriber>();
CopyOnWriteArrayList<Subscriber> refArr = 

cacheUtils.getSubscriberMap().putIfAbsent(syncDet.getCardNumber(), subscriberArr);

if (refArr != null) {
 subscriberArr = refArr;
}

}
 subscriberArr.add(syncDet.getSubScriber());

On iterating the subscriber map i dont see a value object. size is 0 .

Answer 1

You need to first retrieve the appropriate list then populate it. Something like:

List<String> copyArr = testMap.get(id);
if (copyArr == null) {
    copyArr = new CopyOnWriteArrayList<String>();
    List<String> inMap = testMap.putIfAbsent(id, copyArr);
    if (inMap != null) copyArr = inMap; // already in map
}
copyArr.add("Add Value");

That way you only put a new list in the map if there wasn't already one and you add your item to whatever list has made it to the map.

Answer 2

There are a couple of problems with the implementation you made.

Firstly, you are checking whether there is no list for a given key in a non-thread-safe way. It can entirely happen that two threads can execute if (testMap.get(id) == null) before any of them puts the key. This will not cause the key to be overriden per se.

However, both lists are generated, and since you're adding elements to those lists in the unsafe if block prior to the setting of the key, what elements wind up in the actual key->value mapping is anyone's guess.

Also, there's absolutely no need for this:

testMap.put(id,testMap.get(id).add("Append Value"));

The list instance is already in the map in this case, you just need to get it and add the value. Note that this may also mess up your previous key assignment!

---

The second, potential problem is that you're using a CopyOnWriteList which creates a new backing array on adding new elements. Two consequences here:

• it's expensive if there are a lot of additions.
• since the add operation is synchronized (through a ReentrantLock), but get isn't, you may get different list contents in different threads for a short time (the list is eventually consistent for addition, however). This is actually by design - CopyOnWriteArrayList is geared towards hi-read/lo-write operations.

You've got at least two ways out here:

• conduct put operations in a thread-safe manner, i.e.
  • use only putIfAbsent.
  • don't add any values to the local copy of the list, just the one you take from get.
• if you need absolute instead of eventual consistency, don't use a CopyOnWriteArrayList at all. Use a normal list with "manual" synchronization instead. You can use e.g. Guava's Multimap's, such as this one, with a synchronization wrapper to save you the trouble (the Javadoc explains how).