8325179: Race in BasicDirectoryModel.validateFileCache

[aivanov-jdk]

trafficstars

Ensure access to the filesLoader field of BasicDirectoryModel is synchronized.

Without synchronization, a thread checks if filesLoader is not null and creates a new FilesLoader thread. If the thread is pre-empted between these two operations, another thread or even several threads can see the null value and create new FilesLoader threads.

The same way, BasicDirectoryModel.invalidateFileCache needs to be synchornized to interrupt the current filesLoader and assign null.

This bug allows reproducing ConcurrentModificationException seen in JDK-8323670 and JDK-8307091 using the test in PR #18109.

---

Progress

• [x] Change must not contain extraneous whitespace
• [x] Commit message must refer to an issue
• [x] Change must be properly reviewed (2 reviews required, with at least 1 Reviewer, 1 Author)

Issues

• JDK-8325179: Race in BasicDirectoryModel.validateFileCache (Bug - P3)
• JDK-8238169: BasicDirectoryModel getDirectories and DoChangeContents.run can deadlock (Bug - P4)

Reviewers

• Phil Race (@prrace - Reviewer) ⚠️ Review applies to a9ec7f64
• Tejesh R (@TejeshR13 - Committer) ⚠️ Review applies to a9ec7f64
• Andrey Turbanov (@turbanoff - Committer) ⚠️ Review applies to a9ec7f64

Reviewing

Using git

Checkout this PR locally:
$ git fetch https://git.openjdk.org/jdk.git pull/18111/head:pull/18111
$ git checkout pull/18111

Update a local copy of the PR:
$ git checkout pull/18111
$ git pull https://git.openjdk.org/jdk.git pull/18111/head

Using Skara CLI tools

Checkout this PR locally:
$ git pr checkout 18111

View PR using the GUI difftool:
$ git pr show -t 18111

Using diff file

Download this PR as a diff file:
https://git.openjdk.org/jdk/pull/18111.diff

Webrev

Link to Webrev Comment

[bridgekeeper[bot]]

:wave: Welcome back aivanov! A progress list of the required criteria for merging this PR into master will be added to the body of your pull request. There are additional pull request commands available for use with this pull request.

[openjdk[bot]]

@aivanov-jdk The following label will be automatically applied to this pull request:

• client

When this pull request is ready to be reviewed, an "RFR" email will be sent to the corresponding mailing list. If you would like to change these labels, use the /label pull request command.

[aivanov-jdk]

/reviewers 2

[mlbridge[bot]]

Webrevs

• 03: Full - Incremental (478cb500)
• 02: Full - Incremental (a9ec7f64)
• 01: Full - Incremental (a0659236)
• 00: Full (4215925b)

[openjdk[bot]]

@aivanov-jdk The total number of required reviews for this PR (including the jcheck configuration and the last /reviewers command) is now set to 2 (with at least 1 Reviewer, 1 Author).

[aivanov-jdk]

/issue add 8238169

[openjdk[bot]]

@aivanov-jdk Adding additional issue to issue list: 8238169: BasicDirectoryModel getDirectories and DoChangeContents.run can deadlock.

[aivanov-jdk]

This fix also resolves JDK-8238169 because the cancelRunnables method is removed; the synchronized modifier from DoChangeContents.run is also removed.

[aivanov-jdk]

@mrserb, could you take a look, please?

[openjdk[bot]]

@aivanov-jdk This change now passes all automated pre-integration checks.

ℹ️ This project also has non-automated pre-integration requirements. Please see the file CONTRIBUTING.md for details.

After integration, the commit message for the final commit will be:

8325179: Race in BasicDirectoryModel.validateFileCache
8238169: BasicDirectoryModel getDirectories and DoChangeContents.run can deadlock

Reviewed-by: prr, tr, aturbanov, serb

You can use pull request commands such as /summary, /contributor and /issue to adjust it as needed.

At the time when this comment was updated there had been 255 new commits pushed to the master branch:

• 3f2e849c54c2a9c55e3b5c9f5a6d3478b83144e3: 8280392: java/awt/Focus/NonFocusableWindowTest/NonfocusableOwnerTest.java failed with "RuntimeException: Test failed."
• c901da48e30d53cb8e4e3c1f0584c5f2d3d095f1: 8327098: GTest needs larger combination limit
• 9e32db266e4c3cc9be273fa6b77112832a43ba4a: 8328309: Remove malformed masked shift instruction selection patterns
• fc0472baf9bad298e853bf2ca3d10dc9415272cd: 8328000: Convert /java/awt/im/8154816/bug8154816.java applet test to main
• 85fc47c81af81a595dc88e61454d8ba2d860f301: 8327180: Failed: java/io/ObjectStreamClass/ObjectStreamClassCaching.java#G1
• 569b05addf69698fc93026b4dca69bc6ca0920b6: 8327818: Implement Kerberos debug with sun.security.util.Debug
• dec68d7e36a9436468594416272c44a2afbece8d: 8328234: Remove unused nativeUtils files
• 7734466c4627c51b2c24b4b4dbf6db4184607aa3: 8325871: Move StringTable and SymbolTable rehashing calls
• 9e98118f289e98ca9f3be2a274e0ddf8822aaa7c: 8328177: Move LDFLAGS_JDK[LIB/EXE] to JdkNativeCompilation.gmk
• f3af91815a662b195938b26962a8670a4a692220: 8327945: Inline HasScavengableOops
• ... and 245 more: https://git.openjdk.org/jdk/compare/8f6edd8dc866bf970b7e7b8358f62832887e6e8b...master

As there are no conflicts, your changes will automatically be rebased on top of these commits when integrating. If you prefer to avoid this automatic rebasing, please check the documentation for the /integrate command for further details.

➡️ To integrate this PR with the above commit message to the master branch, type /integrate in a new comment.

[aivanov-jdk]

If this makes that CCME go away I'll be pleased.

@prrace CCME is already gone, Tejesh and I resolved it under JDK-8323670 in #17462.

I am assuming you've run the "full" battery of tests on this.

Yes, I did. No failures found.

This is an isolated change, it prevents creating several background thread to load files.

[aivanov-jdk]

If this makes that CCME go away I'll be pleased.

@prrace CCME is already gone, Tejesh and I resolved it under JDK-8323670 in #17462.

The BasicDirectoryModel/ConcurrentModification.java test in #18109 usually failed with OutOfMemoryError on Windows because too many background threads were created and they couldn't complete contending on the COM thread. I saw more than 100 BasicDirectoryModel.FilesLoader threads in the debugger.

With this fix, the test passes without OOME on Windows. At each given time, there's no more than one background thread loading the list of files.

[aivanov-jdk]

@mrserb Do you have any comments?

[aivanov-jdk]

/integrate

[openjdk[bot]]

Going to push as commit e66788c16563d343f6cccd2807a251ccc6f9b64a. Since your change was applied there have been 312 commits pushed to the master branch:

• 1496b5de9004c6a9e43ead5f6e9e88362028a7cc: 8327924: Simplify TrayIconScalingTest.java
• 16ed191329b517766313761c8f3450b4a8285658: 8328101: Parallel: Obsolete ParallelOldDeadWoodLimiterMean and ParallelOldDeadWoodLimiterStdDev
• 581b1e29aebd425bade14d2ee46704a16187df5b: 8328402: Implement pausing functionality for the PassFailJFrame
• 75195aab497a2d23548128e03f6887283dcaa7e1: 8328299: Convert DnDFileGroupDescriptor.html applet test to main
• 725d87bbc2abae2ca856d91668f0a494f93fca1c: 8328570: Convert closed JViewport manual applet tests to main
• e41bc42deb22615c9b93ee639d04e9ed2bd57f64: 8327990: [macosx-aarch64] Various tests fail with -XX:+AssertWXAtThreadSync
• 43080173e88c8f53cd54c9096c79f3144007fd97: 8328631: Convert java/awt/InputMethods/InputMethodsTest/InputMethodsTest.java applet test to manual
• 700d2b91defd421a2818f53830c24f70d11ba4f6: 8328401: Convert java/awt/Frame/InitialMaximizedTest/InitialMaximizedTest.html applet test to automated
• bb3e84bd1fd8217fcb319de8a0716e44359e3423: 8328592: hprof tests fail with -XX:-CompactStrings
• ac2f8e5af8c88cd13038b113f82bb7c17a38aa40: 8327994: Update code gen in CallGeneratorHelper
• ... and 302 more: https://git.openjdk.org/jdk/compare/8f6edd8dc866bf970b7e7b8358f62832887e6e8b...master

Your commit was automatically rebased without conflicts.

[openjdk[bot]]

@aivanov-jdk Pushed as commit e66788c16563d343f6cccd2807a251ccc6f9b64a.

:bulb: You may see a message that your pull request was closed with unmerged commits. This can be safely ignored.