RFR: 8325179: Race in BasicDirectoryModel.validateFileCache [v2]

[Alexey Ivanov]

On Wed, 6 Mar 2024 10:43:16 GMT, Tejesh R <tr at openjdk.org> wrote:

>> Alexey Ivanov has updated the pull request incrementally with four additional commits since the last revision:
>> 
>>  - Replace synchronized invalidateFileCache with synchronized block inside
>>  - Declare DoChangeContents constructor private, wrap its parameters
>>  - Space after synchronized in DoChangeContents.run
>>  - Convert runnable to local variable
>
> src/java.desktop/share/classes/javax/swing/plaf/basic/BasicDirectoryModel.java line 552:
> 
>> 550:             if (remSize > 0 && addSize == 0) {
>> 551:                 fireIntervalRemoved(BasicDirectoryModel.this, remStart, remStart + remSize - 1);
>> 552:             } else if (addSize > 0 && remSize == 0 && addStart + addSize <= fileCache.size()) {
> 
> Any reason for moving this portion out of `Synchronized` block? Because `fileCache.size()` might need to be inside the `Synchronized` block right?

I didn't touch it, it never was inside the synchronized block. Only indentation was changed in this method.

However, you're right it's better to store the size of `fileCache` inside the synchronized block. The lock should not be held while notifying listeners.

On the other hand, `fileCache` can't change. The only place where `fileCache` is modified is above in this method, and only on EDT. The EDT will remain busy until a `fire*` method returns.

I've updated the code to avoid any confusion. Thanks!

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

PR Review Comment: https://git.openjdk.org/jdk/pull/18111#discussion_r1514311500