ConcurrentMap::compute clarification

[Paul Sandoz]

Hi Jaromir,

Thanks, you found a bug, seems to be a hangover from the Map default implementation.

One fix might be:

if (oldValue != null) {
    // something to remove
    if (remove(key, oldValue)) {
        // removed the old value as expected
        return null;
    }

    // some other value replaced old value. try again.
    oldValue = get(key);
} else if (contains(key)) {
    // a mapping was added after obtaining the old value, try again
    oldValue = get(key);
} else {
    // nothing to do. Leave things as they were.
    return null;
}

Although i am somewhat inclined to do away with the contains check (given nulls are not supported), so things are just left alone.

Do you wanna report the issue here:

  http://bugreport.java.com/

Thanks,
Paul.

> On 8 Dec 2015, at 11:12, Jaromir Hamala <jaromir.hamala at gmail.com> wrote:
> 
> Hi,
> 
> I stumbled upon an interesting issue with default implementation of
> `compute(K key, BiFunction<? super K, ? super V, ? extends V>
> remappingFunction)` in JDK8 `ConcurrentMap`.
> According to its contract the default method implementation assumes map
> implementations do not support null values.
> 
> This is the begin of the default implementation:
> 
> default V compute(K key, BiFunction<? super K, ? super V, ? extends V>
> remappingFunction) {
>        Objects.requireNonNull(remappingFunction);
>        V oldValue = get(key);
>        for(;;) {
>            V newValue = remappingFunction.apply(key, oldValue);
>            if (newValue == null) {
>                // delete mapping
>                if (oldValue != null || containsKey(key)) {
>                    // something to remove
>                    if (remove(key, oldValue)) {
> [...]
> 
> 
> Let's say we have an empty map and 2 threads:
> T1 is calling the `compute('foo', someFunction)`
> T2 is concurrently calling calling `put('foo', 'bar');`
> 
> so the T1 will get `oldValue = null`, but `containsKey()` will return
> `true` - because T2 already created the mapping `foo -> bar`. Hence T1 will
> call `remove('foo', null)` !
> 
> Contract of `remove()` says: `throws NullPointerException if the specified
> key or value is null, and this map does not permit null keys or values
> optional.` -> the T1 will throw NPE.
> Is it a bug in default method impl or do I understand it wrong?
> 
> Cheers,
> Jaromir
> 
> --
> “Perfection is achieved, not when there is nothing more to add, but when
> there is nothing left to take away.”
> Antoine de Saint Exupéry