RFR: 8323670: A few client tests intermittently throw ConcurrentModificationException

[Tejesh R]

On Wed, 24 Jan 2024 15:26:23 GMT, Alexey Ivanov <aivanov at openjdk.org> wrote:

>> 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.

Using `synchronized (fileCache)` inside `ShellFolder.invoke` would be better and one solution right? Than making local copy and again doing sanity checks/changing to `ArrayList`?

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

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