RFR: 8024688: j.u.Map.merge doesn't work as specified if contains key:null pair

[Mike Duigou]

On Oct 8 2013, at 01:27 , Paul Sandoz wrote:

> Hi Mike,
> 
> I am probably going over old ground here...

Not a problem, I don't think this particular aspect has received as much attention as it probably should

> Given that there is ConcurrentMap is there any point in having the defaults on Map detect concurrent modification and barf, or retry

The retry behaviour in particular doesn't seem to be appropriate for the general Map defaults. The concurrent modification detection at this point is mostly a result of (supposed) invariant checks failing.

> or neither of the previous two e.g. putIfAbsent, computeIfPresent and replace respectively i.e. there seems to be inconsistent behaviour here.

Right, the behaviour is not consistent across all of the Map methods. Some do a better job than others of detecting concurrent modification.

> Would it not be better to separate concerns of serial and concurrent access in the defaults of Map and ConcurrentMap?

Yes,

> 
> The defaults on Map could state they are not suitable for maps that can be concurrently modified,

They probably should do so.

> if that is required extend from ConcurrentMap. If function values passed to methods modify the Map then undefined behaviour will result. (I am also wondering if there are currently edge cases e.g. if a function value modifies the source then the re-try loop will never terminate.)

Yes, there are cases where this can happen.

> I realize that is separate from the null behaviour, but perhaps this separate will help clarify null behaviour?

For the ConcurrentMap implementations I've written the defaults to explicitly assume that null values are not supported (there's @implNote documentation of this assumption)

Mike

> 
> Paul.
> 
> On Oct 4, 2013, at 5:35 AM, Mike Duigou <mike.duigou at oracle.com> wrote:
> 
>> Hello all;
>> 
>> This is a changeset which improves the consistency of several Map.merge implementations for handling of null values. 
>> 
>> The existing unit tests hadn't considered several cases where the result of the remapper was not the same as the value. I've restructured the merge tests to be more thorough and systematic this revealed a couple of problems.
>> 
>> http://cr.openjdk.java.net/~mduigou/JDK-8024688/0/webrev/
>> 
>> Like several of the previous patches, this one introduces an alternative default for ConcurrentMap to work around issues involving null values where the handling in the general Map default would be incorrect.
>> 
>> Mike
>