The return value of “putIfAbsent” must be used - really?

SonarQube Server / Community Build
1

I’m using Version 7.4 (build 18908) and I’ve got a ConcurrentMap of ConcurrentMaps like this…

ConcurrentMap<String, ConcurrentMap<K, V>> mapsMap = new ConcurrentHashMap<>();

Now in some method, I would like to prevent NPE by making sure a map exists for a certain key like this…

mapsMap.putIfAbsent(someKey, new ConcurrentHashMap<K, V>());

…so I can safely call stuff like…

mapsMap.get(someKey).put(...);

…without worrying about null values here.

Now, Sonarqube is telling me, that this violates the rule RSPEC-2201…

Return values from functions without side effects should not be ignored
[…] and also on ConcurrentMap.putIfAbsent calls ignored return value.

Is this just SonarQube not detecting that the side effect of the method is enough for me here (and the return value would not add any information) or am I missing an important point about the putIfAbsent contract, perhaps related to concurrency, since it’s explicitly only for ConcurrentMap?

2

I am not familiar with ConcurrentHashMap, but let’s dig and try to understand what exactly is going on.

First of all RSPEC-2201’s description does not sound convincing to me: ConccurentMap.putIfAbsent() does have a side-effect as per its Javadoc documentation.

Let’s continue by having a look at RSPEC-2201’s implementation in IgnoredReturnValueCheck.java. ConccurentMap.putIfAbsent() is being explicitly checked there, so this was done on purpose. Blaming that line brings us to commit 21496d4 which brings us to ticket SONARJAVA-2546. That ticket’s description is weak and does only justifies adding that method “because FindBugs does it”. This brings us to FindBug’s description of its rule RV_RETURN_VALUE_OF_PUTIFABSENT_IGNORED:

The putIfAbsent method is typically used to ensure that a single value is associated with a given key (the first value for which put if absent succeeds). If you ignore the return value and retain a reference to the value passed in, you run the risk of retaining a value that is not the one that is associated with the key in the map. If it matters which one you use and you use the one that isn’t stored in the map, your program will behave incorrectly.

Now this starts to make some sense.

Coming back to your case, as you are not holding a reference to new ConcurrentHashMap<K, V>(), there is no risk for your program to work on inconsistent data as described in FindBugs’ rule description.

So my conclusion is that your code is indeed safe. Additionally, I believe that SONARJAVA-2546 should have been implemented as a separate rule rather than as an extension to RSPEC-2201. I’ll sync with our product team to see how to get those changes in.

Thanks a lot for your feedback @FlorianSchaetz

3 Likes
4

Thanks alot for this extremely thoughtfull analysis and explanation @dbolkensteyn, it was very enlightening.

Additionally, I suggest not limiting this to ConcurrentMap in the new rule, since the basic reasoning is true for any Map-Implementation (and limiting it to ConurrentMap leads to the idea that this has something to do with Concurrency).

1 Like
5
1 Like