On Wed, 24 Jan 2024 15:26:23 GMT, Alexey Ivanov <[email protected]> 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
