RFR: 8323670: A few client tests intermittently throw ConcurrentModificationException [v2]

[Alexey Ivanov]

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