On Tue, 23 Jan 2024 08:41:02 GMT, Andrey Turbanov <[email protected]> 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
