Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

8325179: Race in BasicDirectoryModel.validateFileCache #18111

Closed

Conversation

aivanov-jdk
Copy link
Member

@aivanov-jdk aivanov-jdk commented Mar 4, 2024

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

  • Change must not contain extraneous whitespace
  • Commit message must refer to an issue
  • 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

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

The method is not needed: fid != fetchID.get() cancels it
The method doesn't need to be synchronised as the object isn't
accessed concurrently and all its fields are final.
The value is assigned to 'runnable' on this thread only;
The value is read on this thread only (to pass to invokeLater)
@bridgekeeper
Copy link

bridgekeeper bot commented Mar 4, 2024

👋 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 openjdk bot added the rfr Pull request is ready for review label Mar 4, 2024
@openjdk
Copy link

openjdk bot commented Mar 4, 2024

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

@openjdk openjdk bot added the client client-libs-dev@openjdk.org label Mar 4, 2024
@aivanov-jdk
Copy link
Member Author

/reviewers 2

@mlbridge
Copy link

mlbridge bot commented Mar 4, 2024

Webrevs

@openjdk
Copy link

openjdk bot commented Mar 4, 2024

@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
Copy link
Member Author

/issue add 8238169

@openjdk
Copy link

openjdk bot commented Mar 4, 2024

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

@aivanov-jdk
Copy link
Member Author

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

@aivanov-jdk
Copy link
Member Author

@mrserb, could you take a look, please?

@@ -395,7 +394,7 @@ public DoChangeContents call() {
}
if (!fileCache.equals(newFileCache)) {
if (loadThread.isInterrupted()) {
cancelRunnables();
return null;
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This should've been return null all the way.

Copy link
Contributor

@prrace prrace left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If this makes that CCME go away I'll be pleased.
I am assuming you've run the "full" battery of tests on this.

@openjdk
Copy link

openjdk bot commented Mar 13, 2024

@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:

  • 3f2e849: 8280392: java/awt/Focus/NonFocusableWindowTest/NonfocusableOwnerTest.java failed with "RuntimeException: Test failed."
  • c901da4: 8327098: GTest needs larger combination limit
  • 9e32db2: 8328309: Remove malformed masked shift instruction selection patterns
  • fc0472b: 8328000: Convert /java/awt/im/8154816/bug8154816.java applet test to main
  • 85fc47c: 8327180: Failed: java/io/ObjectStreamClass/ObjectStreamClassCaching.java#G1
  • 569b05a: 8327818: Implement Kerberos debug with sun.security.util.Debug
  • dec68d7: 8328234: Remove unused nativeUtils files
  • 7734466: 8325871: Move StringTable and SymbolTable rehashing calls
  • 9e98118: 8328177: Move LDFLAGS_JDK[LIB/EXE] to JdkNativeCompilation.gmk
  • f3af918: 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.

@openjdk openjdk bot added the ready Pull request is ready to be integrated label Mar 13, 2024
@aivanov-jdk
Copy link
Member Author

aivanov-jdk commented Mar 13, 2024

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
Copy link
Member Author

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
Copy link
Member Author

@mrserb Do you have any comments?

@aivanov-jdk
Copy link
Member Author

/integrate

@openjdk
Copy link

openjdk bot commented Mar 21, 2024

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

  • 1496b5d: 8327924: Simplify TrayIconScalingTest.java
  • 16ed191: 8328101: Parallel: Obsolete ParallelOldDeadWoodLimiterMean and ParallelOldDeadWoodLimiterStdDev
  • 581b1e2: 8328402: Implement pausing functionality for the PassFailJFrame
  • 75195aa: 8328299: Convert DnDFileGroupDescriptor.html applet test to main
  • 725d87b: 8328570: Convert closed JViewport manual applet tests to main
  • e41bc42: 8327990: [macosx-aarch64] Various tests fail with -XX:+AssertWXAtThreadSync
  • 4308017: 8328631: Convert java/awt/InputMethods/InputMethodsTest/InputMethodsTest.java applet test to manual
  • 700d2b9: 8328401: Convert java/awt/Frame/InitialMaximizedTest/InitialMaximizedTest.html applet test to automated
  • bb3e84b: 8328592: hprof tests fail with -XX:-CompactStrings
  • ac2f8e5: 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 openjdk bot added the integrated Pull request has been integrated label Mar 21, 2024
@openjdk openjdk bot closed this Mar 21, 2024
@openjdk openjdk bot removed ready Pull request is ready to be integrated rfr Pull request is ready for review labels Mar 21, 2024
@openjdk
Copy link

openjdk bot commented Mar 21, 2024

@aivanov-jdk Pushed as commit e66788c.

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

@aivanov-jdk aivanov-jdk deleted the 8325179-race-BasicDirectoryModel branch March 21, 2024 16:14
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
client client-libs-dev@openjdk.org integrated Pull request has been integrated
5 participants