jdk
jdk copied to clipboard
openjdk
8327137: Add test for ConcurrentModificationException in BasicDirectoryModel
I'm adding a regression test for JDK-8323670 and JDK-8307091; it's also a regression test for JDK-8240690.
I referenced this test in PR #17462 in this comment. I fine-tuned the test since that time.
The test doesn't fail all the time without the fix for JDK-8323670, however, it fails sometimes. If you run the test several times, it will likely fail without the fix.
For me, the test fails about 10 times from 40 runs in the CI. It fails on macOS more frequently than on Linux.
When the test passes, it usually completes in 5 minutes.
How the test works
The test creates a temporary directory in the current directory and creates a number of files in it. (The number of files is controlled by NUMBER_OF_THREADS constant). Then the test creates JFileChooser in the temporary directory.
The test starts several scanner threads, the number is controlled by NUMBER_OF_THREADS, which repeatedly call fileChooser.rescanCurrentDirectory(). This results in calling BasicDirectoryModel.validateFileCache which starts a background thread — "Basic L&F File Loading Thread" — to enumerate the files.
A timer is used to create new files in the directory that the file chooser is using.
After enumerating the files, the File Loading Thread posts an event to EDT. The event updates fileCache and fires a ListDataEvent.
If the File Loading Thread is iterating over fileCache using Iterator (when fileCache.subList or fileCache.equals is running; or a new Vector instance is created from a fileCache or its sublist) and fileCache is being updated on EDT, then ConcurrentModificationException is thrown.
On Linux and on headless macOS, ShellFolder.invoke is executed in the caller, which makes it easier to reproduce the issue. Because of JDK-8325179, there are several File Loading Threads, which also helps to reproduce the issue.
On headful macOS, the BasicDirectoryModel is not used, so the test does not reproduce the issue.
On Windows, the test does not fail or fails with OutOfMemoryError. It is because all the File Loading Threads are serialised on the COM thread, ShellFolder.invoke submits the task to the COM thread and waits for it to complete. The chance of updating fileCache while another thread is iterating over it is significantly reduced.
Even more, filechooser.isTraversable(file) is also executed on the COM thread, which means there's a heavy contention for the COM thread from each File Loading Thread.
Progress
- [ ] Change must be properly reviewed (1 review required, with at least 1 Reviewer)
- [x] Change must not contain extraneous whitespace
- [x] Commit message must refer to an issue
Issue
- JDK-8327137: Add test for ConcurrentModificationException in BasicDirectoryModel (Bug - P4)
Reviewing
Using git
Checkout this PR locally:
$ git fetch https://git.openjdk.org/jdk.git pull/18109/head:pull/18109
$ git checkout pull/18109
Update a local copy of the PR:
$ git checkout pull/18109
$ git pull https://git.openjdk.org/jdk.git pull/18109/head
Using Skara CLI tools
Checkout this PR locally:
$ git pr checkout 18109
View PR using the GUI difftool:
$ git pr show -t 18109
Using diff file
Download this PR as a diff file:
https://git.openjdk.org/jdk/pull/18109.diff
Webrev
: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.
![bridgekeeper[bot] avatar](https://avatars.githubusercontent.com/u/41768318?v=4 "Published by a pub.dev verified publisher"){kind=small}
@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.
@mrserb, @RealCLanger, could you run this test in your environments, please? It is for Linux and macOS only.
I'm looking for confirmation that the test fails without the fix for JDK-8323670. Since JDK-8323670 is fixed in mainline, you can run it in 21u or 17u which don't have the fix yet.
You'll need to run the test several times because it fails only occasionally. That's why I want to collect more data on whether the test is useful/stable enough.
I greatly appreciate your help.
"When the test passes, it usually completes in 5 minutes."
and you've never seen the test time out ? I thought default jtreg timeout is 120 seconds.
"When the test passes, it usually completes in 5 minutes."
and you've never seen the test time out ? I thought default jtreg timeout is 120 seconds.
No, I've never seen the test time out.
The default timeout is factored by 2 in client jobs, another factor could be applied on the hosts.
@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:
8327137: Add test for ConcurrentModificationException in BasicDirectoryModel
Reviewed-by: serb, tr
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 444 new commits pushed to the master branch:
- 5cddc2de493d9d8712e4bee3aed4f1a0d4c228c3: 8325252: C2 SuperWord: refactor the packset
- 6b1b0e9d45eb56f88398e2a6bca0d90c03112eaa: 8329103: assert(!thread->in_asgct()) failed during multi-mode profiling
- bc546c21a59d2481ba86f98d0d653c7691f68d4c: 8328561: test java/awt/Robot/ManualInstructions/ManualInstructions.java isn't used
- af7c6af0cc1eb6c42199c05933c7feb032bd6353: 8324808: Manual printer tests have no Pass/Fail buttons, instructions close set 3
- d3fc8df8af11d7cc1cc341bc75e46b7e93d6db31: 8329135: Store Universe::*exception_instance() in CDS archive
- a85c8493aec73e81c000ea3e3d983b05706bbfec: 8328273: sun/management/jmxremote/bootstrap/RmiRegistrySslTest.java failed with java.rmi.server.ExportException: Port already in use
- 70c8ff1c9a9adf21a16d8a6b4da1eeec65afe61d: 8328665: serviceability/jvmti/vthread/PopFrameTest failed with a timeout
- ecd2b7112a7617b11997da341d6185756bf1fb0f: 8329354: java/text/Format/MessageFormat/CompactSubFormats.java fails
- c2979c150bdbcc2a9e6026347dc590e6a7e86595: 8329425: ProblemList containers/docker/TestJFREvents.java on linux-x64
- 5698f7ad29c939b7e52882ace575dd7113bf41de: 8329134: Reconsider TLAB zapping
- ... and 434 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.
@mrserb, @RealCLanger, could you run this test in your environments, please? It is for Linux and macOS only.
I'm looking for confirmation that the test fails without the fix for JDK-8323670. Since JDK-8323670 is fixed in mainline, you can run it in 21u or 17u which don't have the fix yet.
You'll need to run the test several times because it fails only occasionally. That's why I want to collect more data on whether the test is useful/stable enough.
I greatly appreciate your help.
I added it to our testing. Results early next week.
I added it to our testing. Results early next week.
@RealCLanger Thank you. I'm more interested in the failing case. How reliable does the test reproduce the problem?
I added it to our testing. Results early next week.
@RealCLanger Thank you. I'm more interested in the failing case. How reliable does the test reproduce the problem?
Yes, it's running in 21 and 22 as well, so we'll see what happens there.
I added it to our testing. Results early next week.
@RealCLanger Thank you. I'm more interested in the failing case. How reliable does the test reproduce the problem?
Yes, it's running in 21 and 22 as well, so we'll see what happens there.
We got the following, once in jdk21u and once in jdk21u-dev on macintelx64 through the weekend:
2024-03-16 05:55:37.407 java[3056:1287194577] XType: Using static font registry. 2024-03-16 05:55:38.047 java[3056:1287194577] name is : .SFNS-Regular 2024-03-16 05:55:38.047 java[3056:1287194577] family is : .AppleSystemUIFont 2024-03-16 05:55:38.047 java[3056:1287194577] name is : .SFNS-Bold 2024-03-16 05:55:38.047 java[3056:1287194577] family is : .AppleSystemUIFont 2024-03-16 05:55:38.616 java[3056:1287194577] nsFont-name is : .AppleSystemUIFont 2024-03-16 05:55:38.616 java[3056:1287194577] nsFont-family is : .AppleSystemUIFont 2024-03-16 05:55:38.616 java[3056:1287194577] nsFont-desc-name is : .SFNS-Regular 2024-03-16 05:55:38.616 java[3056:1287194577] nsFont-name is : .AppleSystemUIFont 2024-03-16 05:55:38.617 java[3056:1287194577] nsFont-family is : .AppleSystemUIFontBold 2024-03-16 05:55:38.617 java[3056:1287194577] nsFont-desc-name is : .SFNS-Bold 2024-03-16 05:55:38.629 java[3056:1287194577] nsFont-name is : .AppleSystemUIFont 2024-03-16 05:55:38.629 java[3056:1287194577] nsFont-family is : .AppleSystemUIFont 2024-03-16 05:55:38.629 java[3056:1287194577] nsFont-desc-name is : .SFNS-Regular 2024-03-16 05:55:38.629 java[3056:1287194577] nsFont-name is : .AppleSystemUIFont 2024-03-16 05:55:38.629 java[3056:1287194577] nsFont-family is : .AppleSystemUIFontBold 2024-03-16 05:55:38.629 java[3056:1287194577] nsFont-desc-name is : .SFNS-Bold Exception in Basic L&F File Loading Thread: class java.util.ConcurrentModificationException java.util.ConcurrentModificationException at java.base/java.util.AbstractList$SubList.checkForComodification(AbstractList.java:906) at java.base/java.util.AbstractList$SubList.size(AbstractList.java:792) at java.base/java.util.AbstractCollection.toArray(AbstractCollection.java:143) at java.base/java.util.Collections$SynchronizedCollection.toArray(Collections.java:2310) at java.base/java.util.Vector.
(Vector.java:182) at java.desktop/javax.swing.plaf.basic.BasicDirectoryModel$FilesLoader$1.call(BasicDirectoryModel.java:365) at java.desktop/javax.swing.plaf.basic.BasicDirectoryModel$FilesLoader$1.call(BasicDirectoryModel.java:342) at java.desktop/sun.awt.shell.ShellFolderManager$DirectInvoker.invoke(ShellFolderManager.java:148) at java.desktop/sun.awt.shell.ShellFolder.invoke(ShellFolder.java:532) at java.desktop/sun.awt.shell.ShellFolder.invoke(ShellFolder.java:518) at java.desktop/javax.swing.plaf.basic.BasicDirectoryModel$FilesLoader.run0(BasicDirectoryModel.java:342) at java.desktop/javax.swing.plaf.basic.BasicDirectoryModel$FilesLoader.run(BasicDirectoryModel.java:295) at java.base/java.lang.Thread.run(Thread.java:1583)
So it seems to work that the test can reproduce the issue.
Yes, it's running in 21 and 22 as well, so we'll see what happens there.
We got the following, once in jdk21u and once in jdk21u-dev on macintelx64 through the weekend:
Thank you, Christoph! @RealCLanger. At least one failure is detected. It's better than nothing, isn't it?
Yes, it's running in 21 and 22 as well, so we'll see what happens there.
We got the following, once in jdk21u and once in jdk21u-dev on macintelx64 through the weekend:
Thank you, Christoph! @RealCLanger. At least one failure is detected. It's better than nothing, isn't it?
Definitely helpful. For these fixes - JDK-8323670 and JDK-8307091, the only way to debug was with code walkthrough. This test will help a lot even its able to reproduce intermittently according to me, especially to build some confidence for the fix.
Going to push as commit 9731b1c8b02d957985f4fb40acd93fb67747a9f0.
Since your change was applied there have been 577 commits pushed to the master branch:
- c5150c7b81e2b7b8c9e13c228d3b7bcb9dfe5024: 8309751: Duplicate constant pool entries added during default method processing
- 86cb76728dd164faa8fe69cd07db85977e79be29: 8326568: jdk/test/com/sun/net/httpserver/bugs/B6431193.java should use try-with-resource and try-finally
- b49ba426a721db5926ac1b45d573d468389d479c: 8330002: Remove redundant public keyword in BarrierSet
- dd6e4533eb8b9c33b03a041d7a9ac87160ff9ffb: 8329767: G1: Move G1BlockOffsetTable::set_for_starts_humongous to HeapRegion
- e0fd6c4c9e30ef107ea930c8ecc449842ae4b8d4: 8329545: [s390x] Fix garbage value being passed in Argument Register
- 51ed69a586105b707ae616f9eba898449bf9fba7: 8327621: Check return value of uname in os::get_host_name
- bea9acc55a7b0463a1b0b4dcb557f8ea17d8fe8c: 8328482: Convert and Open source few manual applet test to main based
- d037a597a94edf6e716098b88f42f2b15518e2bd: 8311248: Refactor CodeCache::initialize_heaps to simplify adding new CodeCache segments
- bab70193ddaae66a1f9039b0f1af1a0dc0e39cf9: 8329431: Improve speed of writing CDS heap objects
- 47df14590c003ccb1607ec0edfe999fcf2aebd86: 8310513: [s390x] Intrinsify recursive ObjectMonitor locking
- ... and 567 more: https://git.openjdk.org/jdk/compare/8f6edd8dc866bf970b7e7b8358f62832887e6e8b...master
Your commit was automatically rebased without conflicts.
@aivanov-jdk Pushed as commit 9731b1c8b02d957985f4fb40acd93fb67747a9f0.
:bulb: You may see a message that your pull request was closed with unmerged commits. This can be safely ignored.