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

[Brent Christian]

On 2/5/15 12:46 AM, Paul Sandoz wrote:
> On Feb 5, 2015, at 1:36 AM, Brent Christian
> <brent.christian at oracle.com> wrote:
>> I prefer this approach of discouraging/preventing side-effects via
>> CME, rather than allowing them.
>> ...
>> Regarding the default methods:
>> Would we be able to make a "best-effort" detection of
>> comodification...
 >>
> My inclination is to leave these as is and focus on the most common
> concrete implementations.

Fair enough.  So, spec-wise...

For Map, I'd like to add some "words of discouragement" regarding 
modifying the map from function code.  Maybe also encourage implementing 
classes to detect and throw CME? (The default method will not do so.)

For HashMap: update the doc to say concurrent mods will results in a 
CME, add @throws.

For ConcurrentMap: It does not discourage modifying the map from 
function code, but it should IMO.  It should not encourage subclasses to 
throw CME.

ConcurrentHashMap: Does it need any changes?  It already mentions that 
one "must not attempt to update any other mappings of this map."  FWIW, 
giving the side-effecty mappingFunction from 8071667 to a CHM hangs for 
me; I've not investigated. :\

I don't think any changes are needed for concrete Map impls that inherit 
the default Map methods (e.g. TreeMap, IdentityHashMap)

Here's an initial rough cut, for 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


Thanks,
-Brent