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

[Brent Christian]

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).

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."  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