RFR: 8323670: A few client tests intermittently throw ConcurrentModificationException

[Alexey Ivanov]

On Thu, 25 Jan 2024 04:43:59 GMT, Tejesh R <tr at openjdk.org> wrote:

> Instead of putting everything into `synchronized(fileCache)`, I guess the better solution would be to wrap this line
> 
> https://github.com/openjdk/jdk/blob/c702dcabf8befc2db2baf53655f20391ee5d2d09/src/java.desktop/share/classes/javax/swing/plaf/basic/BasicDirectoryModel.java#L365
> 
> into `synchronized(fileCache)`. With this we can ensure that a local copy would be made before comparing two fileCache list, since we cannot/shouldn't depend on any addition/deletion from `fileCache`.

But this won't resolve the problem completely.

A better solution could still be creating a local copy of `fileCache` and then use it to calculating the diffs.

There's still a problem: if `fileCache` is mutated again, the diffs become outdated. It's not taken into account by the current code. We can still ignore it.

Then I haven't figured it out for what the calculated diffs are used. I also see that the evaluation can be cancelled.

In fact, I see more thread-safety issues in the implementation… There are some fields and variables which are accessed without proper synchronisation.


I looked at the code once again after reading @mrserb's [code review](https://mail.openjdk.org/pipermail/swing-dev/2020-March/010163.html) for [JDK-8240690](https://bugs.openjdk.org/browse/JDK-8240690), it seems that it goes like this:

1. The loader thread is created. Its `run0` method populates the files from file system. (It does so using `fileSystem.getFiles` which may be not thread-safe as well.)
2. The list of files is filtered (which also uses possibly non-thread-safe methods from `JFileChooser`).
3. The list of files is sorted.
4. The code inside `ShellFolder.invoke` is run on yet another thread. It calculates the diffs between the current `fileCache` and `newFileCache` created up to step 3.
5. The result of step 4 is an object of `DoChangeContents` which contains a snapshot of added or removed files. The object implements `Runnable` interface and is assigned to the field `runnable`.
6. This `runnable` is posted to be executed on EDT via `invokeLater`. It updates `fileCache`.

Step 6 uses `doFire` variable, it could be set to false using the `cancel` method, which is synchronized. As such, reading the value of `doFire` must also be synchronised.

Looking at the methods of `BasicDirectoryModel`, I can see that some methods use synchronized(fileCache) before accessing the field but others (`getSize`, `contains`, `indexOf`, `getElementAt`), which makes the `BasicDirectoryModel` class not thread-safe; it is especially dangerous with `contains` and `indexOf` where the contents of the `fileCache` can be changed while the method is iterating over the values inside `fileCache`.

-------------

PR Comment: https://git.openjdk.org/jdk/pull/17462#issuecomment-1909906871