value from ConcurrentHashMap throws NullPointerException

Question

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?

I think you are right. But why don't you just debug and prove? — DKD, May 22 '19 at 07:07
I can't debug - it happen randomly, multi-threaded, and the input is only available at the client.. — Eli, May 22 '19 at 07:10
If you can't debug, then you may want to try adding a null check before adding an item to your `mapsList`. If that's working, it proves your assumption is correct. Otherwise, look for any code that could potentially modify the `conMap`. — DKD, May 22 '19 at 07:16

score 1 · Accepted Answer · answered May 24 '19 at 13:36

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) {
            //...
        }
    }
}

value from ConcurrentHashMap throws NullPointerException

1 Answers1