RFC: Adding ConcurrentModificationException for HashMap.computeIfAbsent(), and JDK-8071667

[Paul Sandoz]

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.christian at 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