JDK-8222394 : HashMap clear does ++modCount unconditionally, so do compute/computeIfAbsent throw CME
  • Type: Bug
  • Component: core-libs
  • Sub-Component: java.util:collections
  • Affected Version: 13
  • Priority: P4
  • Status: Closed
  • Resolution: Won't Fix
  • OS: generic
  • CPU: generic
  • Submitted: 2019-04-12
  • Updated: 2019-05-15
  • Resolved: 2019-05-15
Related Reports
Relates :  
JDK-8071667 - HashMap.computeIfAbsent() adds entry that HashMap.get() does not find.
Description
Firstly this is a tricky case triggered in my testing farm. I found it with one of my aarch64 system (Ampere) and have finally proven it as a common problem with a small and "sneaky" case.

Here is the original post:
https://mail.openjdk.java.net/pipermail/core-libs-dev/2019-March/059430.html
Some updates:
https://mail.openjdk.java.net/pipermail/core-libs-dev/2019-April/059729.html

It is known that HashMap does not support concurrent access, but would be responsible of throwing CME "on a best-effort basis" if a potential concurrent modification is detected.


Here is my test case, [0] creates a HashMap, keeps it empty, and calls m.computeIfAbsent() or m.compute(), in which a "sneaky" m.clear() occurs, some of the test cases throw CME although there were no "structural" changes in fact. (A structural modification is defined as "any operation that adds or deletes one or more mappings...").

jdk8u cannot reproduce this issue, but jdk9 and beyond can, since the bug [1] got fixed for computeIfAbsent() concurrent co-modification issues. A couple of test cases [2] were introduced at that time, and the focus was to verify the behaviors at resizing, while empty maps were not tested.

A possible "fix" for this issue is to move the unconditional "modCount++" [3] into the if-clause, which indicates that a "structural" change would be happening indeed.

public void clear() {
    Node<K,V>[] tab;
-   modCount++;
    if ((tab = table) != null && size > 0) {
+        modCount++;
          size = 0;
          for (int i = 0; i < tab.length; ++i)
            tab[i] = null;
        }
}

Therefore, a dilemma here is "modCount++ before-if-clause but overkills some cases" vs. "modCount++ into-if-clause but weakens the CME checking potentially". I want to make a more reasonable balance regarding how to "throw CME on a best-effort basis" more appropriately. Any suggestion?

I understand that CME here in HashMap.java cannot guarantee much and may be only for debugging purpose, any concurrent modification needs to be typically accomplished by synchronizing on some object that naturally encapsulates the map. So the mentioned issue is a just a tricky case.

[0]http://cr.openjdk.java.net/~qpzhang/map.clear/webrev.01/test/jdk/java/util/concurrent/ConcurrentMap/ConcurrentModification.java.udiff.html
[1]https://bugs.openjdk.java.net/browse/JDK-8071667
[2]http://hg.openjdk.java.net/jdk/jdk/file/5a9d780eb9dd/test/jdk/java/util/Map/FunctionalCMEs.java
[3]http://hg.openjdk.java.net/jdk/jdk/file/1042cac8bc2a/src/java.base/share/classes/java/util/HashMap.java#l860
Comments
HashMap.clear() does ++modCount unconditionally on any attempt to modify the collection, rather than on the effective result of that attempt. Bulk modifications like addAll, clear are doing so in other similar container implementations too, while for single-entry modifications like put, remove, merge, etc. it depends, some are doing unconditional ++ and most are conditional ++. This was working very well as it is more defensible, until the jdk9 time frame, additional unconditional modCount checking logic for HashMap.compute() and HashMap.computeIfAbsent() got introduced in order to warn relevant concurrent modification risks. From then on the test case mentioned in this ticket started failing. By far it looks these tests might be over strict, and "fixing" these conflicts would not bring much benefit in consideration of the effort. CMEs are majorly for debugging purpose, and cannot guarantee much, so verifying CMEs would be an invalid or unreliable operation. A suggestion here is adding more comments for this in case anyone else would revisit it with similar confusions. However there are quite a few other cases where the choice of conditional-vs-unconditional is made, duplicated explanations could not be a good update either. In future, documenting this in a common place of javadoc for modCount, or for "fail fast", would be a better choice. Suggest to close this issue if no further comments or follow-ups. https://mail.openjdk.java.net/pipermail/core-libs-dev/2019-May/060084.html
13-05-2019
I think I might also prefer if CME were only thrown conditionally in clear() (or that we got rid of CME entirely - more trouble than it's worth?!) but the current behavior seems deliberate and defensible.
12-04-2019