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

[Mike Duigou]

On Oct 3 2013, at 22:15 , David Holmes wrote:

> On 4/10/2013 1:35 PM, Mike Duigou wrote:
>> Hello all;
>> 
>> This is a changeset which improves the consistency of several Map.merge implementations for handling of null values.
> 
> It isn't at all clear to me what specification you are using to define the expected behaviour here.

Most of the new Map methods provide only limited support for null values. This is not due to null-hostility but generally a consequence of trying to avoid the additional overhead of disambiguating between value absent and null value. This includes the result of get() and null returns are also used from the functions to provide special behaviour (usually removal). 

The merge() spec is typical in that it promises to treat null and absent values the same. Some might suggest this is a cunning plot by Doug to discourage use of null values. Perhaps. ;-) The utility of being able to use null values as sentinels, whether from get() or the use function is primary reason for this behaviour. Making the behaviour of the Map defaults and core collections "not horrible" and "not astonishing" in the presence null values has been a fun challenge. :-) It's sometimes necessitated providing alternative defaults for ConcurrentMap. I believe that the defaults in Map and ConcurrentMap could further diverge over time if it turns out to be a problem that the current Map defaults don't look aggressively enough for concurrent modification.

> I would have thought that anyone supplying a remapping function needs to be aware of whether the target map supports nulls or not, and that the remapping function should then do the right thing if null is encountered. Instead you are making the decision to bypass the remapping function if you encounter a null.

Bypassing the function is what merge() does when there's no existing values. This behaviour is not specific to my changes. Here's the logic table I used to construct the tests:

current	value	merged	put/rem	returned
=======	=====	======	=======	========
absent	null	n/a	remove	null
absent	newval	n/a	newval	newval
null	null	n/a	remove	null
null	newval	n/a	newval	newval
oldval	null    null	remove	null
oldval	null	result	result	result
oldval	newval	null	remove	null
oldval	newval	result	result	result

Alternatively, compute() offers the more general behaviour of always calling the remapping function though it does not offer the opportunity to directly provide a "newValue" (capture by the lambda is the alternative). 

If I had designed the API I would have had only a single method that provided newValue and unconditionally called the function. This is probably would have been naive--I don't know Doug's reasons behind choosing this behaviour for merge() and compute() though I am certainly curious about why it is done the way it is.

Mike

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