Wrong implementation of Oracle Java ConcurrentHashMap?

Question

I am testing ConcurrentHashMap on Oracle's Java 8 implementation:

ConcurrentMap<String, String> concurrentMap = new ConcurrentHashMap<>();
String result = concurrentMap.computeIfAbsent("A", k -> "B");
System.out.println(result);  // "B"
result = concurrentMap.putIfAbsent("AA", "BB");
System.out.println(result);  // null

The Javadoc of computeIfAbsent does say that

Implementation Requirements:

The default implementation is equivalent to the following steps for this map, then returning the current value or null if now absent:
if (map.get(key) == null) {
    V newValue = mappingFunction.apply(key);
    if (newValue != null)
        return map.putIfAbsent(key, newValue);
}

It said then returning the current value or null if now absent. So shouldn't it be returning null? Given that putIfAbsent is also returning null.

What am I missing here?

I don't see that this is contradictory with the specification at all — fge, Sep 18 '17 at 05:32
The pseudocode in the Javadoc is meant for illustration. The actual implementation, while similar, handles your case properly. — Joe C, Sep 18 '17 at 05:34
The documentation has been fixed in java 9. See [the bug](http://bugs.java.com/bugdatabase/view_bug.do?bug_id=8174087) and the [fixed version of the javadoc](http://download.java.net/java/jdk9/docs/api/java/util/concurrent/ConcurrentMap.html#computeIfAbsent-K-java.util.function.Function-) — Misha, Sep 18 '17 at 07:54
@Misha Thanks for pointing that out, wonderful. No more debate :) — user1589188, Sep 18 '17 at 08:10

Holger · Accepted Answer · 2017-09-18T08:36:13.467

The code example of ConcurrentMap.computeIfAbsent is not reflecting the actual intention, most likely a mistake caused by the non-intuitive behavior of putIfAbsent, while the implementation obeys the documented intention. This has been reported in JDK-8174087 and fixed in Java 9

Note that the contract for Map.computeIfAbsent is

Implementation Requirements:

The default implementation is equivalent to the following steps for this map, then returning the current value or null if now absent:
if (map.get(key) == null) {
    V newValue = mappingFunction.apply(key);
    if (newValue != null)
        map.put(key, newValue);
}

omitting the return statement. But clearly says

Returns:

the current (existing or computed) value associated with the specified key, or null if the computed value is null

It is the documentation of ConcurrentMap.computeIfAbsent that tries to incorporate the concurrency aspect, falling for the non-inuitive behavior of putIfAbsent:

Implementation Requirements:

The default implementation is equivalent to the following steps for this map, then returning the current value or null if now absent:
if (map.get(key) == null) {
    V newValue = mappingFunction.apply(key);
    if (newValue != null)
        return map.putIfAbsent(key, newValue);
}

but it still says

Returns:

the current (existing or computed) value associated with the specified key, or null if the computed value is null

and the documented intention should have precedence over a code example. Note that the actual default implementation of ConcurrentMap.computeIfAbsent is in line with the documented intention:

@Override
default V computeIfAbsent(K key,
        Function<? super K, ? extends V> mappingFunction) {
    Objects.requireNonNull(mappingFunction);
    V v, newValue;
    return ((v = get(key)) == null &&
            (newValue = mappingFunction.apply(key)) != null &&
            (v = putIfAbsent(key, newValue)) == null) ? newValue : v;
}

So the implementation of ConcurrentHashMap.computeIfAbsent does conform to the documented intention of both, ConcurrentMap.computeIfAbsent and Map.computeIfAbsent regarding the returned value and is also equivalent to the default implementation provided by the interfaces.

For completeness, the default implementation of Map.computeIfAbsent is

default V computeIfAbsent(K key,
        Function<? super K, ? extends V> mappingFunction) {
    Objects.requireNonNull(mappingFunction);
    V v;
    if ((v = get(key)) == null) {
        V newValue;
        if ((newValue = mappingFunction.apply(key)) != null) {
            put(key, newValue);
            return newValue;
        }
    }

    return v;
}

Thanks. However, besides the code example, the doc also said above the code: "then returning the current value or null if now absent". Which contradicts to its behaviour "returning the current value or new value if now absent". — user1589188, Sep 18 '17 at 07:26
@user1589188: “*the current value*” is the value stored in the map when the method returns, which might be either “*existing or computed*”, as stated. “*if now absent*” means “if the mapping has been removed upon return”, which can only happen if your function returned `null`, so in that case, the returned value is also identical to the computed value. — Holger, Sep 18 '17 at 07:30
@Holger I can't agree. As `computeIfAbsent` does not remove entry even if the computed/new value is null (unlike `compute` or `merge`). So "if now absent" is referring to the state before calling the method. — user1589188, Sep 18 '17 at 07:44
@user1589188: it doesn’t matter whether there is an entry mapping to `null` or no entry. In either case `computeIfAbsent` (note “compute—if—absent”) will evaluate your function and use the returned value. If your function returns `null`, the next `computeIfAbsent` for the same key will again evaluate the function (consider the key as absent). In case of `ConcurrentMap`s which don’t allow `null` values, returning `null` *does* even imply that there will be no mapping stored. It wasn’t be removed, though, simply because there was no mapping to begin with… — Holger, Sep 18 '17 at 07:52
@Holger there won't be a case that before calling the method there is a current value and after calling the method the value becomes absent. It should not even try to compute if there is a current value to begin with. So saying "if now absent" would be meaningless if it is referring to the state after as "it has always been absent" the whole time. Anyway, they seem to have realised their fault and now fixed in java 9, which is great :) — user1589188, Sep 18 '17 at 08:23
@user1589188: “if now absent” does *not* imply “if it became absent”. And the phrase “*Returns: the current (existing or computed) value* …” is quiet clear. There is room for improvement, indeed, but I still don’t understand where your quibbling is heading at. It looks like you *want* to misunderstand that specification… — Holger, Sep 18 '17 at 08:32

score 0 · Answer 2 · answered Sep 18 '17 at 05:44

0

Actual code from javadoc:

 if (map.get(key) == null) {
 V newValue = mappingFunction.apply(key);
     if (newValue != null)
         map.put(key, newValue);  // <-
     }
 }

As you can see there is no return keyword in the marked line.

Section "return" also says:

Returns: the current (existing or computed) value associated with the specified key, or null if the computed value is null

answered Sep 18 '17 at 05:44

Zefick

2,014
15
19

Hummm, what version is yours? Mine has return exactly as I have copied. I am on 8u60. – user1589188 Sep 18 '17 at 05:52
I have 8_05, 65 and 91. They all have the same javadoc. Here it is: https://docs.oracle.com/javase/8/docs/api/java/util/Map.html#computeIfAbsent-K-java.util.function.Function- – Zefick Sep 18 '17 at 06:35
1

Ah I see. You are reading from Map. But I am referring to ConcurrentMap http://docs.oracle.com/javase/8/docs/api/java/util/concurrent/ConcurrentMap.html#computeIfAbsent-K-java.util.function.Function- – user1589188 Sep 18 '17 at 07:10

Wrong implementation of Oracle Java ConcurrentHashMap?

2 Answers2

Implementation Requirements:

Returns:

Implementation Requirements:

Returns:

Linked