RFR: 8004518 & 8010122 : Default methods on Map

[Mike Duigou]

On Apr 10 2013, at 22:06 , David Holmes wrote:

> Hi Mike,
> 
> Comments inline and trimmed ...

Additional trimming applied.

> On 11/04/2013 2:41 PM, Mike Duigou wrote:
>> On Apr 8 2013, at 19:22 , David Holmes wrote:
>>>> 8004518: Add in-place operations to Map
>>>>  forEach()
>>>>  replaceAll()
>>> 
>>> Both of those contain the boilerplate text:
>>> 
>>>  * <p>The default implementation makes no guarantees about
>>>  * synchronization or atomicity properties of this method. Any
>>>  * class overriding this method must specify its concurrency
>>>  * properties. In particular, all implementations of
>>>  * subinterface {@link java.util.concurrent.ConcurrentMap}
>>>  * must ensure that this operation is performed atomically.
>>> 
>>> but these methods are not, and can not be, atomic in ConcurrentMap
>> 
>> I've altered the text to incorporate your suggestions. It now read:
>> 
>>      * <p>The default implementation makes no guarantees about synchronization
>>      * or atomicity properties of this method. Any class which wishes to provide
>>      * specific synchronization, atomicity or concurrency behaviour should
>>      * override this method.
>> 
>> I would like to use this same text on the other methods as well.
> 
> I'm okay with that, but you should check with whomever thought it necessary to call out the ConcurrentMap situation. ConcurrentMap will need to override these just to add the spec that they are atomic.

I shall. ConcurrentMap already mentions that it's operations are atomic. The Concurrent.getOrDefault() default implementation doesn't appear to need a comment.

> 
>>> forEach doesn't declare the IllegalStateException that getKey and getValue can throw.
>> 
>> Since forEach isn't supposed to mutate the map this shouldn't happen. It could only happen through concurrent modification. I've added a catch of the IllegalStateException and throw CME with the ISE as the cause to both forEach and replaceAll.
> 
> Not sure I see the distinction. These are Map methods so regardless of which method is involved you will only get the IllegalStateException if there is concurrent modification. Hence to me forEach and replaceAll should behave the same way, for example.

OK, check the webrev when I post it. They are now consistent.

> 
>>> Some/many of the @throws text has obviously been copied from the Map.Entry methods eg:
>>> 
>>> * @throws ClassCastException if the class of the specified value
>>> *         prevents it from being stored in the backing map
>>> 
>>> but when put into Map itself they should not be referring to "the backing map" as there is no "backing map". Further we have inconsistent terminology being used, eg getOrDefault has:
>>> 
>>> * @throws ClassCastException if the key is of an inappropriate type for
>>> * this map
>>> 
>>> and then there is a third variant in other methods:
>>> 
>>>  * @throws ClassCastException if the class of the specified key or value
>>>  *         prevents it from being stored in this map
>>>  *         (<a href="Collection.html#optional-restrictions">optional</a>)
>>> 
>>> These should all have the same basic wording, differing only in key/value.
>> 
>> agreed. The third variant is now used consistently.
> 
> Why do we need the xref to "optional" ? Is throwing CCE optional? Other Collection methods don't flag it as optional.

Since this method is applied to collections who's underlying implementation may or may not throw CCE we have to document that it's optional. Without the "optional" it would be required to throw the CCE in the described condition and we can't guarantee that the map will do that.

> 
>>>>  compute()              *
>>>>  merge()                *
>>>>  computeIfAbsent()      *
>>>>  computeIfPresent()     *
>>> 
>>> The following generally apply to this group of methods.
>> 
>>> I find the spec for these rather confusing from a concurrency perspective - this non-concurrent interface seems to be trying to say too much about how a concurrent interface should specify behaviour. Why does it need to say:
>>> 
>>>  * In concurrent contexts, the default implementation may retry
>>>  * these steps when multiple threads attempt updates.
>> 
>> For default on Map I am starting to think that throwing that CME rather than looping is the right choice. The retry behaviour seems to be counter the basic behaviour of non-concurrent implementations. The retry behaviour will just hide usage that's otherwise unsafe when used with non-concurrent implementations.
>> 
>> The concurrent implementations don't use these defaults so there's no problem switching to throwing CME.

>> 
>> Opinions?
> 
> I had assumed the loops only existed because you wanted to use them as ConcurrentMap defaults too! If that is not the case then these methods should not loop/retry but just throw CME, in my opinion.

Correction (of my previous statement). ConcurrentMap reabstracts only the defaults for putIfAbsent, remove(Object,Object), replace(K,V), replace(K,V,V). The defaults are used compute, computeIfAbsent, computeIfPresent and merge.

I am inclined to move these implementations as defaults to ConcurrentMap and replace the defaults in Map with ones that throw CME.

Mike.