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

[Paul Sandoz]

Hi Mike,

This looks like a good direction. Need to look more closely.

I wonder if we even require such detailed support to throw CME? 

For the cases where no function values are passed it has very limited value, we know it is effectively useless for non-current collections being modified concurrently.

For the cases where a function value is passed and it modifies the map then it could have some value. But i wonder, for any non-looping function value accepting method on Map (anything other than forEach/replaceAll), whether it is just simpler to state: "if a function value modifies the entry under computation then this method may return incorrect results`".

Paul.

On Oct 10, 2013, at 5:46 AM, Mike Duigou <mike.duigou at oracle.com> wrote:

> 
> On Oct 8 2013, at 01:27 , Paul Sandoz wrote:
> 
>> Hi Mike,
>> 
>> I am probably going over old ground here...
>> 
>> Given that there is ConcurrentMap is there any point in having the defaults on Map detect concurrent modification and barf, or retry, or neither of the previous two e.g. putIfAbsent, computeIfPresent and replace respectively i.e. there seems to be inconsistent behaviour here.
>> 
>> Would it not be better to separate concerns of serial and concurrent access in the defaults of Map and ConcurrentMap?
> 
> I have created a prototype renovated Map and ConcurrentMap to provide (mostly) separate implementations.
> 
> http://cr.openjdk.java.net/~mduigou/JDK-8024688.2/0/webrev/
> 
> This passes all of the existing regression tests and I believe is performance neutral. The implementations are now more relatively correct for both flavours. ie. Map defaults don't retry and throw CME when they detect that something changed unexpectedly. The ConcurrentMap defaults were mostly just moving over the prior Map defaults but I also removed a few cases of null value handling that were no longer needed.
> 
> I've wanted to explore this for a while (since June) but haven't previously had a chance. I'm somewhat convinced that this is the right direction to be going but I am sure further refinement is possible (particularly in regards to the spec).
> 
>> The defaults on Map could state they are not suitable for maps that can be concurrently modified, 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.)
>> 
>> I realize that is separate from the null behaviour, but perhaps this separate will help clarify null behaviour?
> 
> I realized that some of the statements about "implementations must document how they handle nulls with this method" were no longer relevant. The null handling behaviour was entirely covered by the class documentation and the method specification. This is mostly a consequence of having added "or is mapped to null value" in couple of cases later on in the process.
> 
> Thanks for pushing on this point (and David Holmes for earlier suggestion that this might be important)
> 
> 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
>> 
>