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

[David Holmes]

Okay you have incited me to throw in my 2c :) I think the CME issue has 
been raised a number of times in the past (and if the below doesn't 
agree with what I've said in the past Hey! It's a brand new day! ;-) )

But first, Mike the missing spaces are creeping back in "if(xxx)" :)

For Map I don't think looking for external concurrent modification and 
throwing CME is necessary or worthwhile. These are not thread-safe 
methods. That covers:
  - remove, replace

and it implies that putIfAbsent should not check for or throw CME.

For compute* and merge it is possible that the computation function 
modifies the Map - unlikely perhaps but possible - so CME here seems 
more reasonable. (As for forEach, replaceAll etc.)

I fully agree with removing the retry loops in these non-concurrent 
implementations.

That said it makes ConcurrentMap somewhat different to Map as it never 
throws CME even if it was an internal mutation.

Aside: do you still need to re-list every unchecked exception when 
overriding (i.e. @throws foo {@inheritDoc}) to get them to show up in 
the subtype docs? I can't tell if this has been forgotten or whether the 
ConcurrentMap methods truly will never throw such exceptions.

Cheers,
David
-----

On 16/10/2013 2:38 AM, Paul Sandoz wrote:
>
> On Oct 15, 2013, at 6:25 PM, Mike Duigou <mike.duigou at oracle.com> wrote:
>
>>
>> On Oct 14 2013, at 02:32 , Paul Sandoz wrote:
>>
>>>> Virtually all the cases where CME is thrown in the "new" Map defaults are the points where previously the implementation would have looped/retried.
>>>>
>>>
>>>>> 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.
>>>>
>>>> Understood. The alternative would be to GIGO these situations, return and ignore them.
>>>>
>>>
>>> Yeah, unnecessarily complicates the code.
>>
>> It does detect legitimate errors and it makes me uncomfortable to just ignore them.
>>
>
> It is not deterministic.
>
> Does HashMap's implementation of putIfAbsent throw a CME?
>
>
>>>>> 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`".
>>>>
>>>> Modification of any other entry may have the same result.
>>>
>>> Yes, although IIUC modification, by the function value, of other entries will not interfere with that of operating on the entry under computation. A more general recommendation is the function values should be stateless.
>>>
>>>
>>>> I suspect that modification by some other thread is as likely to be a problem as modification by the function.
>>>>
>>>
>>> And under concurrent modification we cannot deterministically detect. CME in the non-concurrent collections is only useful for detecting serial modification due to inversion of control, and in these particular cases it is really very limited as to what it detects.
>>
>> True, but it generally doesn't impose any extra cost. The error detection happens as a side effect of necessary operations.
>>
>
> There cost to code complexity. I would argue that additional complexity is not worth it given the limits of what can be detected.
>
> I guess you know my opinion: remove 'em. So i will be silent for a bit and see if anyone else speaks up :-)
>
> Paul.
>
>> I'd like to bring this set of changes to conclusion as soon as possible.
>>
>> Mike
>>
>