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
