Hi Brent,
On the Map.compute* methods.
Perhaps we can reuse similar language to that we added for Matcher:
The mapping function should not modify this map during computation. This
method will, on a best-effort basis, throw a
ConcurrentModification if such modification is detected.
It's tempting to place the second sentence on the @throws
ConcurrentModificationException as i believe will not show up for methods on
ConcurrentMap, so keeps it somewhat contained documentation wise.
On Mar 6, 2015, at 8:08 PM, Brent Christian <brent.christ...@oracle.com> wrote:
> Hi. I'm picking this back up now.
>
> In Map/HashMap, there are already two methods that accept lambdas and throw a
> CME:
> forEach(BiConsumer)
> replaceAll(BiFunction)
>
> The Map interface documents these methods to state that "Exceptions are
> relayed to the caller", and has an @throws CME tag "if an entry is found to
> be removed". (The default code does not throw a CME if items are added).
>
Right, although the iterator might still be fail-fast.
> HashMap adds no additional documentation on these methods. It has code to
> check the modCount, and throws a CME if it changes.
>
>
> The Map/HashMap methods I would like to update with this fix are:
> compute(Key, BiFunction)
> computeIfAbsent(Key, Function)
> computeIfPresent(Key, BiFunction)
> merge(Key, Value, BiFunction)
>
> I would like to update the docs & code as follows:
> Map:
> Docs: discourage modifying the map from function code; encourage
> implementing classes to detect/throw CME
> Code: no change
>
> HashMap/Hashtable:
> Docs: document that structural modifications will result in a CME; add
> @throws CME
> Code: check modCount & throw CME
>
> ConcurrentMap:
> Docs: discourage modifying the map from function code
> Code: no change
>
>
> My first draft for the docs change, on computeIfAbsent():
>
> diff -r 330dcd651f3b src/java.base/share/classes/java/util/Map.java
> --- a/src/java.base/share/classes/java/util/Map.java Mon Feb 02 12:35:18
> 2015 -0800
> +++ b/src/java.base/share/classes/java/util/Map.java Fri Feb 06 12:49:19
> 2015 -0800
> @@ -925,6 +925,11 @@
> * }
> * }</pre>
> *
> + * <p>The mappingFunction itself should not make changes to this map.
> + * Implementing classes are encouraged to detect such modifications and
> + * throw ConcurrentModificationException. The default implementation does
> + * not do so.
> + *
> * <p>The default implementation makes no guarantees about synchronization
> * or atomicity properties of this method. Any implementation providing
> * atomicity guarantees must override this method and document its
>
> diff -r 330dcd651f3b src/java.base/share/classes/java/util/HashMap.java
> --- a/src/java.base/share/classes/java/util/HashMap.java Mon Feb 02
> 12:35:18 2015 -0800
> +++ b/src/java.base/share/classes/java/util/HashMap.java Fri Feb 06
> 12:49:19 2015 -0800
> @@ -1082,6 +1082,17 @@
> return null;
> }
>
> + /**
> + * {@inheritDoc}
> + *
> + * <p>The mappingFunction itself should not make changes to this map.
> + * If the function causes a structural modification to the map, a
> + * ConcurrentModificationException will be thrown. As with iterators,
> this
> + * exception is thrown on a best-effort basis.
> + *
> + * @throws ConcurrentModificationException if a structural change was
> + * detected while executing the mappingFunction
> + */
> @Override
> public V computeIfAbsent(K key,
> Function<? super K, ? extends V>
> mappingFunction) {
>
> diff -r 330dcd651f3b
> src/java.base/share/classes/java/util/concurrent/ConcurrentMap.java
> --- a/src/java.base/share/classes/java/util/concurrent/ConcurrentMap.java
> Mon Feb 02 12:35:18 2015 -0800
> +++ b/src/java.base/share/classes/java/util/concurrent/ConcurrentMap.java
> Fri Feb 06 12:49:19 2015 -0800
> @@ -305,6 +305,8 @@
> * threads attempt updates including potentially calling the mapping
> * function multiple times.
> *
> + * <p>The mappingFunction itself should not make changes to this map.
> + *
> * <p>This implementation assumes that the ConcurrentMap cannot contain
> null
> * values and {@code get()} returning null unambiguously means the key is
> ---
>
> If that looks okay, I will apply it to the other methods.
>
>
> I came across a few methods used by the list classes that I wanted to point
> out:
>
> forEach(Consumer) on the Iterable interface
> removeIf(Predicate) on the Collection interface
> replaceAll(UnaryOperator) on the List interface
>
> They all document that exceptions are relayed to the caller. They do not
> have a @throws CME tag. The default code uses iterators (or enhanced for()).
> LinkedList uses the default versions. ArrayList overrides the methods,
> adds no docs of its own, and has code to check the modCount and throw CME.
>
> Would an "@throws CME" improve the JavaDoc of these default methods? I'm
> leaning toward, "not really."
Same here, they require traversal so use an iterator or an internal equivalent,
thus the CME pops out due to that.
Paul.
> Between the clause about exceptions being relayed to the caller, and knowing
> that the default method is using iterators (mentioned in the JavaDoc), and
> that iterators will throw CMEs, one can deduce that modifying the list from
> the lambda will result in a CME. It's not clearly spelled out, but then,
> modifying the collection this way falls outside the intended usage of these
> methods.
>
> Thanks for any additional feedback.
>
> -Brent
>
> On 2/6/15 1:12 PM, Brent Christian wrote:
>> diff -r 330dcd651f3b src/java.base/share/classes/java/util/Map.java
>> --- a/src/java.base/share/classes/java/util/Map.java Mon Feb 02
>> 12:35:18 2015 -0800
>> +++ b/src/java.base/share/classes/java/util/Map.java Fri Feb 06
>> 12:49:19 2015 -0800
>> @@ -925,6 +925,11 @@
>> * }
>> * }</pre>
>> *
>> + * <p>The mappingFunction itself should not make changes to this map.
>> + * Implementing classes are encouraged to detect such modifications
>> and
>> + * throw ConcurrentModificationException. The default
>> implementation does
>> + * not do so.
>> + *
>> * <p>The default implementation makes no guarantees about
>> synchronization
>> * or atomicity properties of this method. Any implementation
>> providing
>> * atomicity guarantees must override this method and document its
>>
>> diff -r 330dcd651f3b src/java.base/share/classes/java/util/HashMap.java
>> --- a/src/java.base/share/classes/java/util/HashMap.java Mon Feb
>> 02 12:35:18 2015 -0800
>> +++ b/src/java.base/share/classes/java/util/HashMap.java Fri Feb
>> 06 12:49:19 2015 -0800
>> @@ -1082,6 +1082,17 @@
>> return null;
>> }
>>
>> + /**
>> + * {@inheritDoc}
>> + *
>> + * <p>The mappingFunction itself should not make changes to this map.
>> + * If the function causes a structural modification to the map, a
>> + * ConcurrentModificationException will be thrown. As with
>> iterators, this
>> + * exception is thrown on a best-effort basis.
>> + *
>> + * @throws ConcurrentModificationException if a structural change was
>> + * detected while executing the mappingFunction
>> + */
>> @Override
>> public V computeIfAbsent(K key,
>> Function<? super K, ? extends V>
>> mappingFunction) {
>>
>> diff -r 330dcd651f3b
>> src/java.base/share/classes/java/util/concurrent/ConcurrentMap.java
>> ---
>> a/src/java.base/share/classes/java/util/concurrent/ConcurrentMap.java
>> Mon Feb 02 12:35:18 2015 -0800
>> +++
>> b/src/java.base/share/classes/java/util/concurrent/ConcurrentMap.java
>> Fri Feb 06 12:49:19 2015 -0800
>> @@ -305,6 +305,8 @@
>> * threads attempt updates including potentially calling the mapping
>> * function multiple times.
>> *
>> + * <p>The mappingFunction itself should not make changes to this map.
>> + *
>> * <p>This implementation assumes that the ConcurrentMap cannot
>> contain null
>> * values and {@code get()} returning null unambiguously means the
>> key is
