RFR: 8323670: A few client tests intermittently throw ConcurrentModificationException

[Alexey Ivanov]

On Tue, 23 Jan 2024 08:41:02 GMT, Andrey Turbanov <aturbanov at openjdk.org> wrote:

>> Then adding `synchronized(fileCache)` in `FilesLoader` where list comparison happens would be better idea?
>
> Yes. If you go with `synchronized` then it should be `synchronized (fileCache)`

This is my concern as well. It looks as if synchronisation is missing where it's required and therefore it allows for concurrent modification.

Moreover, when you make a copy of `Vector`, you have to do it in a synchronised block with the `Vector` as the monitor: copying is iterating all the values, and you want to ensure it's done in an atomic way.

Therefore, as @turbanoff noted, equals must be invoked while you hold the object monitor.

It looks as if the previous fix didn't actually fix the problem, because `equals` is still called several times on `fileCache` or with it without proper synchronisation.

It seems the entire `call` that's passed to `ShellFolder.invoke` needs to be inside `synchronized (fileCache)` — its size, its elements are accessed many times while the method is executed. If another thread can modify `fileCache` concurrently, the assumptions inside the code could be broken. The method starts with `oldSize = fileCache.size()` and then iterates over elements in `fileCache` with `i < oldSize`. What if the number of elements in `fileCache` reduces and becomes less than `oldSize`.

Probably, the method should make its own local copy of `fileCache`, do its work using the unsynchronised local copy. When complete, perform another sanity check _inside a synchronized block_ at least to ensure the size hasn't changed and then 

The variables `newFileCache` and `newFiles` could be `ArrayList` instead of `Vector` to avoid synchronisation penalty where it's not needed.

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

PR Review Comment: https://git.openjdk.org/jdk/pull/17462#discussion_r1465088991