RFR: 8323670: A few client tests intermittently throw ConcurrentModificationException [v2]
Alexey Ivanov
aivanov at openjdk.org
Mon Jan 29 19:54:26 UTC 2024
On Thu, 25 Jan 2024 13:58:50 GMT, Tejesh R <tr at openjdk.org> wrote:
>> Suggested fix [JDK-8307091](https://bugs.openjdk.org/browse/JDK-8307091) also created concurrent exception intermittently (monthly once/quarterly once) on CI system. The issue was not able to be reproduced yet, hence proposing an alternative fix which uses iterators to compare the List.
>> CI testing shows green.
>
> Tejesh R 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:
>
> - Revert fix 8307091 + Synchronised filecache
> - Merge branch 'master' of https://git.openjdk.java.net/jdk into branch_8323670
> - Fix
Changes requested by aivanov (Reviewer).
src/java.desktop/share/classes/javax/swing/plaf/basic/BasicDirectoryModel.java line 1:
> 1: /*
Please update copyright year.
src/java.desktop/share/classes/javax/swing/plaf/basic/BasicDirectoryModel.java line 32:
> 30: import java.beans.PropertyChangeSupport;
> 31: import java.io.File;
> 32: import java.util.Iterator;
Unused import.
src/java.desktop/share/classes/javax/swing/plaf/basic/BasicDirectoryModel.java line 344:
> 342: // execute the whole block on the COM thread
> 343: runnable = ShellFolder.invoke(new Callable<DoChangeContents>() {
> 344: public DoChangeContents call() {
To reduce indentation level, you may convert the anonymous class to a lambda expression or even to a method in `FilesLoader` in which case you could use method reference.
src/java.desktop/share/classes/javax/swing/plaf/basic/BasicDirectoryModel.java line 367:
> 365:
> 366: if (start >= 0 && end > start && newFileCache.subList(end, newSize)
> 367: .equals(fileCache.subList(start, oldSize))) {
I suggest following the style used before your first fix:
Suggestion:
if (start >= 0 && end > start
&& newFileCache.subList(end, newSize)
.equals(fileCache.subList(start, oldSize))) {
That is wrap at `&&` operator.
Then wrap before `.equals` to make the line fit into 80-column limit.
src/java.desktop/share/classes/javax/swing/plaf/basic/BasicDirectoryModel.java line 372:
> 370: }
> 371: return new DoChangeContents(newFileCache
> 372: .subList(start, end), start, null, 0, fid);
I don't like such wrapping style. When you read this statement, you miss `newFileCache`, you may think that the call to `subList` is on `DoChangeContents`, which is not the case.
I suggest wrapping after the call to `subList`, that is after the first parameter; or wrap before `newFileCache` so that all the parameters are on the next line.
src/java.desktop/share/classes/javax/swing/plaf/basic/BasicDirectoryModel.java line 387:
> 385:
> 386: if (start >= 0 && end > start && fileCache.subList(end, oldSize)
> 387: .equals(newFileCache.subList(start, newSize))) {
The same as above:
Suggestion:
if (start >= 0 && end > start
&& fileCache.subList(end, oldSize)
.equals(newFileCache.subList(start, newSize))) {
src/java.desktop/share/classes/javax/swing/plaf/basic/BasicDirectoryModel.java line 392:
> 390: }
> 391: return new DoChangeContents(null, 0, new Vector<>(fileCache
> 392: .subList(start, end)), start, fid);
Wrapping at higher level is preferable:
Suggestion:
return new DoChangeContents(null, 0,
new Vector<>(fileCache.subList(start, end)), start, fid);
-------------
PR Review: https://git.openjdk.org/jdk/pull/17462#pullrequestreview-1849605557
PR Review Comment: https://git.openjdk.org/jdk/pull/17462#discussion_r1470124921
PR Review Comment: https://git.openjdk.org/jdk/pull/17462#discussion_r1470101630
PR Review Comment: https://git.openjdk.org/jdk/pull/17462#discussion_r1470123895
PR Review Comment: https://git.openjdk.org/jdk/pull/17462#discussion_r1470107213
PR Review Comment: https://git.openjdk.org/jdk/pull/17462#discussion_r1470111897
PR Review Comment: https://git.openjdk.org/jdk/pull/17462#discussion_r1470117703
PR Review Comment: https://git.openjdk.org/jdk/pull/17462#discussion_r1470119698
More information about the client-libs-dev
mailing list