Java concurrency - use which technique to achieve safety?

Question

I have a list of personId. There are two API calls to update it (add and remove):

public void add(String newPersonName) {
    if (personNameIdMap.get(newPersonName) != null) {
        myPersonId.add(personNameIdMap.get(newPersonName)
    } else {
        // get the id from Twitter and add to the list
    }  
    // make an API call to Twitter
 }

public void delete(String personNAme) {
    if (personNameIdMap.get(newPersonName) != null) {
        myPersonId.remove(personNameIdMap.get(newPersonName)
    } else {
        // wrong person name
    }  
    // make an API call to Twitter
}

I know there can be concurrency problem. I read about 3 solutions:

1. synchronized the method
2. use Collections.synchronizedlist()
3. CopyOnWriteArrayList

I am not sure which one to prefer to prevent the inconsistency.

Answer 1

1) synchronized the method

2) use Collections.synchronizedlist

3) CopyOnWriteArrayList ..

All will work, it's a matter of what kind of performance / features you need.

Method #1 and #2 are blocking methods. If you synchronize the methods, you handle concurrency yourself. If you wrap a list in Collections.synchronizedList, it handles it for you. (IMHO #2 is safer -- just be sure to use it as the docs say, and don't let anything access the raw list that is wrapped inside the synchronizedList.)

CopyOnWriteArrayList is one of those weird things that has use in certain applications. It's a non-blocking quasi-immutable list, namely, if Thread A iterates through the list while Thread B is changing it, Thread A will iterate through a snapshot of the old list. If you need non-blocking performance, and you are rarely writing to the list, but frequently reading from it, then perhaps this is the best one to use.

---

edit: There are at least two other options:

4) use Vector instead of ArrayList; Vector implements List and is already synchronized. However, it's generally frowned, upon as it's considered an old-school class (was there since Java 1.0!), and should be equivalent to #2.

5) access the List serially from only one thread. If you do this, you're guaranteed not to have any concurrency problems with the List itself. One way to do this is to use Executors.newSingleThreadExecutor and queue up tasks one-by-one to access the list. This moves the resource contention from your list to the ExecutorService; if the tasks are short, it may be fine, but if some are lengthy they may cause others to block longer than desired.

In the end you need to think about concurrency at the application level: thread-safety should be a requirement, and find out how to get the performance you need with the simplest design possible.

---

On a side note, you're calling personNameIdMap.get(newPersonName) twice in add() and delete(). This suffers from concurrency problems if another thread modifies personNameIdMap between the two calls in each method. You're better off doing

PersonId id = personNameIdMap.get(newPersonName);
if (id != null){
    myPersonId.add(id);
} 
else 
{
    // something else
}

Answer 2

Collections.synchronizedList is the easiest to use and probably the best option. It simply wraps the underlying list with synchronized. Note that multi-step operations (eg for loop) still need to be synchronized by you.

Some quick things

• Don't synchronize the method unless you really need to - It just locks the entire object until the method completes; hardly a desirable effect
• CopyOnWriteArrayList is a very specialized list that most likely you wouldn't want since you have an add method. Its essentially a normal ArrayList but each time something is added the whole array is rebuilt, a very expensive task. Its thread safe, but not really the desired result