CODEWITHSUNDEEP
javaconcurrencynullpointerexception
Eli
Eli

Reputation: 4936

value from ConcurrentHashMap throws NullPointerException

I'm maintaining multi-threaded legacy code that uses ConcurrentHashMap.

There are operations of add and remove in other methods.

In the following code, at some point after collecting few values from the map, it throws NullPointerException when executing synchronize(value).

public class MyClass{

    private final Map<MyObj, Map<String, List<String>>> conMap = new ConcurrentHashMap<>();

    //...

    public void doSomthing((MyObj id){
        List<Map<String, List<String>>> mapsList = new LinkedList<>();
        for(MyObj objId: conMap.keySet()){              
            if(objId.key1.equals(id.key1)){
                mapsList.add(conMap.get(objId));
            }
        }

        for(Map<String, List<String>> map: mapsList){
            synchronized(map){                   // <-- NullPointerException here
                //...
            }
    }

    //...

}

I have a feeling that maybe during the iteration in the first loop, records are being remove. And when the line: 

mapsList.add(conMap.get(objId));

being executed, objId no longer exist and mapsList adding null and as a result, during the second loop NullPoinerException is thrown.

Is there any other reason to get this exception?

Upvotes: 0

Views: 238

Answers (1)

Holger
Holger

Reputation: 298389

You have fallen for the Check-Then-Act anti-pattern. It implies checking a condition (like the presence of a key), followed by acting upon it (like calling get), ignoring the possibility that the condition may have changed in-between.

So you encounter a particular key when iterating over conMap.keySet(), but by the time you’re invoking conMap.get(objId), the key might not be in the map anymore, which is reported by returning null.

It’s strongly recommended to use a key type having a suitable hashCode/equals implementation, so you don’t need to iterate over the entire map to find matches but can use a single get(id).

However, when you have to iterate over the map and need the values, iterate over the entry set instead of the key set.

public void doSomething(MyObj id){
    // see https://stackoverflow.com/q/322715/2711488
    List<Map<String, List<String>>> mapsList = new ArrayList<>();

    for(Map.Entry<MyObj, Map<String, List<String>>> e: conMap.entrySet()){              
        if(e.getKey().key1.equals(id.key1)){
            mapsList.add(e.getValue());
        }
    }

    for(Map<String, List<String>> map: mapsList){
        synchronized(map) {
            //...
        }
    }
}

Upvotes: 1

Related Questions