Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

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

Closed
spring-projects-issues opened this issue Mar 13, 2018 · 4 comments
Assignees
Labels
in: core status: backported type: bug
Milestone

Comments

@spring-projects-issues
Copy link
Collaborator

spring-projects-issues commented Mar 13, 2018

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:

Referenced from: commits 8d8bb04, 356ef45

Backported to: 4.3.15

@spring-projects-issues
Copy link
Collaborator Author

spring-projects-issues commented Mar 13, 2018

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
Copy link
Collaborator Author

spring-projects-issues commented Mar 14, 2018

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
Copy link
Collaborator Author

spring-projects-issues commented Mar 14, 2018

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
Copy link
Collaborator Author

spring-projects-issues commented Mar 14, 2018

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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
in: core status: backported type: bug
Projects
None yet
Development

No branches or pull requests

2 participants