How to resolve the findbug Sequence of calls to java.util.concurrent.ConcurrentHashMap may not be atomic

Question

Hi I am getting the bug "Sequence of calls to java.util.concurrent.ConcurrentHashMap may not be atomic " when i am running find bug in my project for the below code.

public static final ConcurrentHashMap<String,Vector<Person>> personTypeMap = new ConcurrentHashMap<String, Vector<Person>>();

    private static void setDefaultPersonGroup() {


        PersonDao crud = PersonDao.getInstance();
        List<Person> personDBList = crud.retrieveAll();
        for (Person person : personDBList) {
            Vector<Person> personTypeCollection = personTypeMap.get(person
                    .getGroupId());
            if (personTypeCollection == null) {
                personTypeCollection = new Vector<Person>();
                personTypeMap.put(personTypeCollection.getGroupId(),
                        personTypeCollection);
            }
            personTypeCollection.add(person);
        }
    }

I am facing the problem at the line personTypeMap.put(personTypeCollection.getGroupId(), personTypeCollection);

Can any one help me to resolve the problem.

Answer 1

Compound operations are unsafe in concurrent environment.

What compound operations are you performing?

• 1) You are checking whether Map contains a vector for a key
• 2) You are putting a new Vector if no value is found

So this is a two step action and is compound, so it is unsafe.

Why are they not safe?

Because they are not atomic. Think of a scenario in which you have two threads.

Consider this timeline:

Thread 1 --- checks for == null -> true                                           puts a new Vector

Thread 2 ---                      checks for ==null -> true    puts a new Vector                        

Use putIfAbsent() method on ConcurrentHashMap, which provides you an atomic solution to what you are trying to perform.

ConcurrentHashMap#putIfAbsent()

References:

• Proper use of putIfAbsent

Answer 2

That findbugs message is telling you in the case of multi-threaded access it's not safe:

You're fetching something from personTypeMap, checking to see if it's null, then putting a new entry in if that's the case. Two threads could easily interleave here:

Thread1: get from map
Thread2: get from map
Thread1: check returned value for null
Thread1: put new value
Thread2: check returned value for null
Thread2: put new value

(Just as an example; in reality the ordering is not a given - the point is both threads get null then act on it)

You should be creating a new entry, then calling personTypeMap.putIfAbsent() as this guarantees atomicity.

Answer 3

In your case, your code should looks like this:

public static final ConcurrentHashMap<String,Vector<Person>> personTypeMap = new ConcurrentHashMap<String, Vector<Person>>();

private static void setDefaultPersonGroup() {
    PersonDao crud = PersonDao.getInstance();
    List<Person> personDBList = crud.retrieveAll();
    for (Person person : personDBList) {
        // the putIfAbsent works in the same way of your
        //previous code, but in atomic way
        Vector<Person> personTypeCollection = personTypeMap.putIfAbsent(person
                .getGroupId());
        personTypeCollection.add(person);
    }
}