0

I have a global cache named statisticsCache which is being modified and read by multiple threads simultaneously. Even after I have applied null check but sometime it throws NullPointerException in load run. See below for details:

static Map<String, List<Statistics>> statisticsCache = new ConcurrentHashMap<String, List<Statistics>>();

// method to read the global  cache
List<Statistics> getStatisticsForQueue(String name) {
    List<Statistics> statsCopy = Collections.emptyList();
    List<Statistics> statistics = statisticsCache.get(name);
    if (statistics != null && !statistics.contains(null)) //Here is the check to avoid NPE but sometimes does not works
        statsCopy = new ArrayList<Statistics>(statistics);
    return statsCopy;
}

//method to write into global cache
private void setStatisticsListForQueue(String name) {
    // flushing all pending Last writes of buckets of a queue to DB
    flushStatisticToDB(name);
    if (!statisticsCache.containsKey(name)) {
        statisticsCache.put(name, new ArrayList<Statistics>(1));
    }
    List<Statistics> queueStatisticsList = queueServiceMetaDao
            .findStatisticsByname(name);
    if (queueStatisticsList != null && !queueStatisticsList.isEmpty()) { 
        for (Statistics statistic : queueStatisticsList) {
            // to avoid NPE
            if (statisticsCache.get(name).contains(statistic)) {
                statisticsCache.get(name).remove(statistic);
            }
            statisticsCache.get(name).add(statistic);
        }
    } else {
        statisticsCache.put(name, new ArrayList<Statistics>(1));
    }
}

//method where I am getting NPE 
public long getSize(String name) {
    long size = 0L;
    List<Statistics> statistics = getStatisticsForQueue(name);
    for (Statistics statistic : statistics) {
        size += statistic.getSize(); //Sometimes it throws NullPointerException
    }
    return size;
}

What preventive check should I apply to avoid this?

Laxmikant
  • 1,551
  • 1
  • 13
  • 30

3 Answers3

2

Even after I have applied null check but sometime it throws NullPointerException in load run.

OK, so if you have multiple threads executing this code, then (IMO) the most likely explanation is that the code is not synchronizing properly. Sure, the map itself is a ConcurrentHashMap, so therefore should be thread-safe. However, you have multiple threads creating, accessing and modifying the ArrayLists without any mutual exclusion or other synchronization on the lists.

There are a variety of things that could go wrong. One possibility is that one thread removes an element from a list, and a second thread is simultaneously calling getSize() on the same list. One possible outcome is that the iteration in getSize() will see a stale value of the list's size, and return an array element that has been nulled by the other thread's removal. Since there is no synchronization of the two threads' operations on the list, "all bets are off" with respect to the visibility of one thread's list changes to the other thread.

No matter what exact mechanism leads to the NPEs, what you are doing here does not meet the JLS requirements (see JLS 17.4) that must be met to guarantee predictable behavior.

What preventive check should I apply to avoid this?

You can't solve the problem that way. You need proper synchronization on the lists for ensure that reads and updates cannot overlap. You also need to use putIfAbsent rather than if (! containsKey) { put ... } to deal with another race condition.

Stephen C
  • 698,415
  • 94
  • 811
  • 1,216
0

I think that statistic.getSize() might be null so you are trying to do:

size += statistic.getSize();

Which throws a NullPointerException

You should check if all Statistics objects have their property "size" != null

dvelle
  • 1
  • 1
  • but size is long . How come it can give NPE. – Laxmikant Dec 02 '15 at 12:52
  • @user2492242 If the `Long` is used it can be `null`, so if your `Statistics.getSize()` has return type `Long` and it is never set it may be `null` and will indeed cause a NPE. – Ian2thedv Dec 02 '15 at 13:54
  • but it is not Long public long getCount() { return count; } public void setCount(long count) { this.count = count; } public long getSize() { return size; } public void setSize(long size) { this.size = size; } – Laxmikant Dec 02 '15 at 19:54
0

The problem is actually not the getSize() method since long cannot be null. The actually NPE is this

 List<Statistics> statistics = getStatisticsForQueue(name);

for (Statistics statistic : statistics) 

If statistics is null the for loop will have a NPE. So what you could do to avoid that is

if(statistics != null)
  for (Statistics statistic : statistics) 
Murat Karagöz
  • 35,401
  • 16
  • 78
  • 107
  • I also think that the statistic is null but as you can see in getStatisticsForQueue i have already applied a preventive chek: if (statistics != null && !statistics.contains(null)) //Here is the check to avoid NPE but sometimes does not works – Laxmikant Dec 02 '15 at 12:45
  • I assume !statistics.contains(null) check will make sure that there is no null entry in the cache – Laxmikant Dec 02 '15 at 12:55