8327137: Add test for ConcurrentModificationException in BasicDirectoryModel #18109
Conversation
This test creates 10 threads which concurrently call rescanFileSystem that calls BasicDirectoryModel.validateFileCache.
Make the timer create a random number of files and more frequently.
Ensure the test cleans up after itself: * Handle exceptions in the threads and timer task, * Interrupt threads in this case, * Pass the first exception as the test result, * Add as suppressed exceptions if more exceptions occur.
This reverts commit a7de790.
It's more jtreg friendly, files are created in the scratch directory which is cleaned up automatically. Otherwise, the directory could be left on the system when the test failed. ConcurrentModificationException is thrown in a thread where this test doesn't catch the exception. Mark timeStart as final.
When all the test threads are created within a new ThreadGroup, uncaughtException can catch ConcurrentModificationException which occurs in the 'Basic L&F File Loading Thread'. In this case, the test exits quickly and cleans up the files. Restructure the source file: main, wrapper, runTest; exception handlers; helper classes. Change the test parameters.
All the 8323670, 8307091, 8240690 are about the same problem where ConcurrentModificationException is thrown from BasicDirectoryModel.
On Windows, the test usually fails with OutOfMemoryException. Because ShellFolder.invoke() runs on COM thread on Windows, all the (worker) threads are serialised on COM thread, reducing the opportunity to concurrently modify fileCache. On Linux and *headless* macOS, ShellFolder.invoke() runs in the same thread, therefore all the threads run simultaneously.
It's important to do it on macOS, otherwise the test won't fail because BasicDirectoryModel isn't used in headful environment on macOS.
|
👋 Welcome back aivanov! A progress list of the required criteria for merging this PR into |
|
@aivanov-jdk The following label will be automatically applied to this pull request:
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. |
Webrevs
|
|
@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. |
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: 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
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 |
| try { | ||
| createFiles(temp); | ||
|
|
||
| final JFileChooser fc = new JFileChooser(temp.toFile()); |
There was a problem hiding this comment.
it should be created on EDT, just in case.
There was a problem hiding this comment.
I don't think it matters here… The test does not rely on even processing on EDT, no state of JFileChooser is modified except for calling rescanCurrentDirectory. Technically, I am not allowed to do so either. But if I call rescanCurrentDirectory from EDT only, the test becomes void.
There was a problem hiding this comment.
I think it will fire the property change events via firePropertyChange from the constructor and it is better to do that on EDT.
There was a problem hiding this comment.
I guess it does fire property change events on the current thread rather than EDT in this case. I still don't think it matters in this case. After the JFileChooser is created, it is accessed on the Scanner threads only, and only to call its rescanCurrentDirectory method. This call, in its turn, creates a new background thread which accesses the fields and methods of JFileChooser as well as FileSystemView.
Instantiating JFileChooser on EDT adds complexity for no benefit. The JFileChooser is still accessed concurrently from several threads, including Swing internal thread.
There was a problem hiding this comment.
Instantiating JFileChooser on EDT adds complexity for no benefit. The JFileChooser is still accessed concurrently from several threads, including Swing internal thread.
But still, this is a common requirement to create UI components on EDT, and that will replicate more or less how it will work in the application.
There was a problem hiding this comment.
Instantiating JFileChooser on EDT adds complexity for no benefit. The JFileChooser is still accessed concurrently from several threads, including Swing internal thread.
But still, this is a common requirement to create UI components on EDT, and that will replicate more or less how it will work in the application.
And it is a common requirement not to call fileChooser.rescanCurrentDirectory concurrently in 5 different threads. So it does not apply.
There was a problem hiding this comment.
In general, I agree all Swing components are to be created and accessed on EDT.
However, there are cases where instantiating and testing is safe on main thread. It is when the component is not shown on the screen, its state does not depend on any events dispatched by the event queue. There are such tests in the JDK code base.
I think this test is one of such cases where JFileChooser can be (safely) created on the current thread. The test does not display the file chooser, its state does not depend on receiving input events and handling them.
Essentially, the test verifies that the implementation of JFileChooser.rescanCurrentDirectory is thread-safe. It wasn't. Two bugs were resolved to make the implementation thread-safe: JDK-8323670 and JDK-8325179.
There was a problem hiding this comment.
However, there are cases where instantiating and testing is safe on main thread.
That is my point, make it less safe - when the component is created on one thread:EDT and then for some reason accessed on a different thread, the rescanCurrentDirectory should still work.
There was a problem hiding this comment.
However, there are cases where instantiating and testing is safe on main thread.
That is my point, make it less safe - when the component is created on one thread:EDT and then for some reason accessed on a different thread, the rescanCurrentDirectory should still work.
I got your idea!
However, I still see no benefit for doing it.
Yes, when JFileChooser is instantiated, it creates its UI delegates and the default model, BasicDirectoryModel. The constructor of the model calls validateFileCache which starts the background thread and will update fileCache.
It does not make the initialisation less thread-safe: the same two threads are involved — FilesLoader and EDT. Yet there's no race yet…
The ConcurrentModificationException is thrown when the background FilesLoader is iterating over fileCache while EDT runs DoChangeContents.run which adds elements to fileCache or removes elements from it.
By the time the Scanner threads start, a race becomes possible. If the initial FilesLoader completes and posts the DoChangeContents object to EDT, the FilesLoader threads created by the scanners will race… but fileCache is empty initially, thus iterating over it is very quick.
Therefore getting ConcurrentModificationException is unlikely until fileCache contains a list of files. This state is reached later in the timeline of the test.
The initial FilesLoader thread that's started when JFileChooser is instantiated plays a role too… Yet it's running concurrently with the Scanner threads whether the JFileChooser object is created on the current thread or on EDT.
Taking the above into account, instantiating JFileChooser on EDT adds complexity to the test but brings no benefits.
test/jdk/javax/swing/plaf/basic/BasicDirectoryModel/ConcurrentModification.java
Show resolved
Hide resolved
test/jdk/javax/swing/plaf/basic/BasicDirectoryModel/ConcurrentModification.java
Show resolved
Hide resolved
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:
So it seems to work that the test can reproduce the issue. |
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. |
|
/integrate |
|
Going to push as commit 9731b1c.
Your commit was automatically rebased without conflicts. |
openjdk
bot
removed
ready
|
@aivanov-jdk Pushed as commit 9731b1c. 💡 You may see a message that your pull request was closed with unmerged commits. This can be safely ignored. |
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_THREADSconstant). Then the test createsJFileChooserin the temporary directory.The test starts several scanner threads, the number is controlled by
NUMBER_OF_THREADS, which repeatedly callfileChooser.rescanCurrentDirectory(). This results in callingBasicDirectoryModel.validateFileCachewhich 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
fileCacheand fires aListDataEvent.If the File Loading Thread is iterating over
fileCacheusingIterator(whenfileCache.subListorfileCache.equalsis running; or a newVectorinstance is created from afileCacheor its sublist) andfileCacheis being updated on EDT, thenConcurrentModificationExceptionis thrown.On Linux and on headless macOS,
ShellFolder.invokeis 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
BasicDirectoryModelis 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.invokesubmits the task to the COM thread and waits for it to complete. The chance of updatingfileCachewhile 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
Issue
Reviewers
Reviewing
Using
gitCheckout this PR locally:
$ git fetch https://git.openjdk.org/jdk.git pull/18109/head:pull/18109$ git checkout pull/18109Update a local copy of the PR:
$ git checkout pull/18109$ git pull https://git.openjdk.org/jdk.git pull/18109/headUsing Skara CLI tools
Checkout this PR locally:
$ git pr checkout 18109View PR using the GUI difftool:
$ git pr show -t 18109Using diff file
Download this PR as a diff file:
https://git.openjdk.org/jdk/pull/18109.diff
Webrev
Link to Webrev Comment