ConcurrentMap::compute clarification

[Jaromir Hamala]

Hi Paul,

many thanks for confirmation. I'm going to fill the bug.

Cheers,
Jaromir

On Wed, Dec 9, 2015 at 1:21 PM, Paul Sandoz <paul.sandoz at oracle.com> wrote:

> 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
>
>


-- 
“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