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

[David Holmes]

On 16/10/2013 9:26 PM, Paul Sandoz wrote:
>
> On Oct 16, 2013, at 12:28 PM, David Holmes <david.holmes at oracle.com> wrote:
>
>> On 16/10/2013 8:03 PM, Paul Sandoz wrote:
>>>
>>> On Oct 16, 2013, at 6:41 AM, David Holmes <david.holmes at oracle.com> wrote:
>>>
>>>> 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.
>>>>
>>>
>>> HashMap.compute*/merge methods do not throw CME either. I suppose those methods could and do so beyond that of only the entry under computation. I think this really points to the fact that, for non-traversal, only concrete implementations are capable of reliably detecting a CME and therefore it's best to leave it up to those implementations should they choose to do so.
>>
>> Perhaps HashMap's implementations should throw CME?
>>
>
> Perhaps, seems to be going beyond the call of duty. My inclination is not to bother. It becomes most relevant with forEach since the consumer will have side-effects that might make it easier to unintentionally slip in a modification to the map itself.

I think there is a lot to be said for consistency. At present it seems 
we don't have a clear idea on how these methods should be spec'd or how 
the implementations should behave.

>> But the possibility of CME has to be allowed for in the spec of the interfaced methods regardless.
>>
>
> Ideally by not say anything :-) otherwise perhaps a variant of the following:

Not saying anything doesn't permit CME to be thrown.

> "If a function value passed to an operation of a non-concurrent map modifies the contents of that map then the result of that operation is undefined.  An implementation may throw {@link ConcurrentModificationException} in such cases and if so that behaviour should be documented."

I don't think it is the Java spirit to allow for undefined behaviour. 
Wouldn't:

@throws CME if the <function> modifies the map and this is detected by 
the implementation

give the same flexibility while not being so obviously flimsy? I prefer 
to see exception info on methods as much as practical - with NPE being 
the obvious exception (no pun intended).

David
-----

> Paul.
>