On Tue, 6 Jun 2023 08:40:43 GMT, Pavel Rappo <pra...@openjdk.org> wrote:
>> Although its name might suggest otherwise,
>> java.util.ConcurrentModificationException (CME) is not necessarily thrown in
>> multithreaded context. For example, a map might throw CME if it detects
>> modification in a midst of an operation which is supposed to be exclusive:
>>
>> jshell> Map<Integer, Integer> m = new HashMap<>();
>> ...> m.computeIfAbsent(0, key -> m.put(1, 2));
>> m ==> {}
>> | Exception java.util.ConcurrentModificationException
>> | at HashMap.computeIfAbsent (HashMap.java:1229)
>> | at (#2:1)
>>
>> Somewhat similar happens in the reproducer: a computation of a tree path for
>> a package element eventually modifies the very map that the result of that
>> computation is to be put in. The map detects that and throws CME.
>>
>> Here are more details. The tree path is to be found for the `bar` package
>> element. The tree path is computed and put into the cache in one go
>> (HashMap.computeIfAbsent). Usually that works fine, but not this time.
>> Because the `bar` package resides in an implicitly referred module
>> (test.bar), that package is discovered late. The computation triggers the
>> `bar` package completion/parsing, which eventually calls back to
>> JavadocEnter.visitTopLevel, which in turn puts an alternatively computed
>> tree path for the `bar` package into that same cache. When the call stack
>> returns to computeIfAbsent, that cache modification is detected and CME is
>> thrown.
>>
>> I can see three obvious ways of fixing this bug:
>>
>> 1. Replace computeIfAbsent with an equivalent combination of simpler
>> methods.
>> 2. Use a map whose computeIfAbsent can withstand non-exclusive
>> modifications.
>> 3. Restructure the code so it avoids such modifications altogether.
>>
>> (3) Is quite risky/complicated and not really needed at this time. (2) While
>> it's unlikely to have noticeable performance overhead, ConcurrentHashMap
>> feels like symptomatic treatment that is also out of place given that
>> JavaDoc is single-threaded.
>>
>> (1) seems the most reasonable. One potential problem with (1) though is that
>> future refactoring may reintroduce the issue again, but in a way that the
>> regression test won't catch.
>
> Pavel Rappo has updated the pull request with a new target base due to a
> merge or a rebase. The incremental webrev excludes the unrelated changes
> brought in by the merge/rebase. The pull request contains three additional
> commits since the last revision:
>
> - Merge branch 'master' into 8304878
> - Check exceptions explicitly
> - Initial commit
While the tests are running one last time before integration, I'll take a
moment to note what might not have been obvious from the diff: computeIfAbsent
in the original code was overkill.
If we reached computeIfAbsent, the map was guaranteed NOT to contain the key.
Using computeIfAbsent was either a typo/remnant from something else or stemmed
from a misconception of how it works. The conditionality of that compound
operation does not generally apply to checking whether the key it is about to
enter has already somehow appeared as the result of call to the mapping
function.
-------------
PR Comment: https://git.openjdk.org/jdk/pull/14310#issuecomment-1578255480
