10

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.

Murali
  • 341
  • 2
  • 7
  • 24

3 Answers3

14

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:

Community
  • 1
  • 1
Narendra Pathai
  • 41,187
  • 18
  • 82
  • 120
  • Hi when i am using putifabsent then find bug throw the error like "Return value of putIfAbsent is ignored, but personTypeCollection is reused in setDefaultPersonGroup() – Murali Jan 21 '14 at 07:13
  • putIfAbsent returns a value, which can be null if the key was not mapped to anything or previous value for that key. So you should use the return value. – Narendra Pathai Jan 21 '14 at 07:14
  • Welcome. You may also see answer of @Bohemian in http://stackoverflow.com/questions/10743622/concurrenthashmap-avoid-extra-object-creation-with-putifabsent – Narendra Pathai Jan 21 '14 at 07:23
3

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.

Brian Roach
  • 76,169
  • 12
  • 136
  • 161
  • Hi when i am using putifabsent() then find bug throw the error like "Return value of putIfAbsent is ignored, but personTypeCollection is reused in setDefaultPersonGroup() – Murali Jan 21 '14 at 07:14
0

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);
    }
}
Nilo Garcia Silveira
  • 1