Inconsistent handling of null values through Java 8 accessors in ConcurrentReferenceHashMap [SPR-16584]

[spring-projects-issues]

Christoph Strobl opened SPR-16584 and commented

ConcurrentReferenceHashMap needs to override the default implementation of computeIfAbsent in order to allow adding computed null values.

#from: ConcurrentMap.java

... implementation assumes that the ConcurrentMap cannot contain null values and get() returning null unambiguously means the key is absent. Implementations which support null values must override this default implementation.

The following currently fails:

ConcurrentReferenceHashMap<String, Object> map = new ConcurrentReferenceHashMap<>();
map.computeIfAbsent("key", key -> null);

assertThat(map.containsKey("key")).isTrue();

---

Affects: 4.3.14, 5.0.4

Issue Links:

• LinkedCaseInsensitiveMap doesn't implement getOrDefault properly [SPR-13981] #18553 LinkedCaseInsensitiveMap doesn't implement getOrDefault properly
• LinkedCaseInsensitiveMap cannot access locale from subclass [SPR-15752] #20307 LinkedCaseInsensitiveMap cannot access locale from subclass
• ConcurrentReferenceHashMap does not enforce visibility of cached EntrySet [SPR-16994] #21532 ConcurrentReferenceHashMap does not enforce visibility of cached EntrySet

Referenced from: commits 8d8bb04, 356ef45

Backported to: 4.3.15

[spring-projects-issues]

Juergen Hoeller commented

The Map.computeIfAbsent javadoc says: If the function returns null no mapping is recorded.

So it looks like the above code should not do anything to begin with. The real need for overriding the default implementation seems to be in detecting whether there is an existing entry: not even triggering the computation if there is an existing null value associated already. The same applies to other new methods on ConcurrentMap. We'll need to restore the semantics from the default implementations on the plain Map interface while retaining atomicity as far as possible.

[spring-projects-issues]

Juergen Hoeller commented

From my perspective, those callback semantics of computeIfAbsent and its siblings computeIfPresent and co do not really make sense for null values to begin with. It seems feasible to keep our existing behavior as-is, defining "absent" and "present" as non-null for the purposes of those methods in our usage context.

However, there is a related bug that needs to be resolved: namely getOrDefault which in its default ConcurrentMap implementation doesn't react to null properly (replacing the default Map implementation which handles null but isn't atomic). Providing a dedicated getOrDefault implementation in ConcurrentReferenceHashMap is clearly necessary here. I'll repurpose this ticket for it.

[spring-projects-issues]

Christoph Strobl commented

Thank you Juergen Hoeller for clarification, totally missed the doc in Map. It still feels a bit strange to not allow put for computed null when the actual implementation allows null. The callback IMHO made sense in so far as I'd be able to delay expensive computation (like a reflective method lookup) that might return null and keeping track of having done the computation already.

[spring-projects-issues]

Juergen Hoeller commented

Some methods are indeed a bit inconsistent in the semantics but that comes from the very Map interface itself, and the somewhat non-obvious overriding in ConcurrentMap (both semantically and in the redefined default implementations on that sub-interface).

Ultimately my guidance is the behavior of a plain HashMap (which allows for null keys and null values as well), and unfortunately the callback semantics for computeIfAbsent and co are limited there, so we can't really make them behave differently in our ConcurrentReferenceHashMap.