-
Notifications
You must be signed in to change notification settings - Fork 5.5k
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
8323670: A few client tests intermittently throw ConcurrentModificationException #17462
Conversation
👋 Welcome back tr! A progress list of the required criteria for merging this PR into |
@TejeshR13 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
|
What is the code path which modifies the vector when we iterate it? |
I don't think we are able to trace it out, since the issue intermittent and previously I had made a copy of the vector list before checking for equality of the list. There was again an issue in the code which I used to copy to a temporary vector. So now instead of using |
@@ -412,6 +413,18 @@ private void cancelRunnables() { | |||
runnable.cancel(); | |||
} | |||
} | |||
|
|||
private synchronized <T> boolean compareIterators(Iterator<T> iterator1, Iterator<T> iterator2) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm not sure I understand, how this synchronized
helps to avoid the issue.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Since concurrent modification exception is thrown, it is clear that the List is being modified while comparing two list. Hence instead of copying the list locally, I have used iterators and comparing element by element in a synchronized
method which ensures single thread is accessing the iterators. Without synchronized
I guess it would again cause concurrentModificationException
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Vector.iterator and Vector.subList.iterator are still check for modification on iteration (see usages of the method java.util.AbstractList.SubList#checkForComodification
). It means, if vector was concurrently modified during iteration - iteration will fail with the ConcurrentModificationException
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, which is why I am using Synchronized to handle concurrentModificationException
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It doesn't work like this. Modification happen in another thread in another method. This synchronized
doesn't affect another method.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Using synchronized (fileCache)
inside ShellFolder.invoke
would be better and one solution right? Than making local copy and again doing sanity checks/changing to ArrayList
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Using
synchronized (fileCache)
insideShellFolder.invoke
would be better and one solution right? Than making local copy and again doing sanity checks/changing toArrayList
?
I'm afraid I can't answer this question without more details on what is achieved by this code. We need to look closely into what was done for JDK-8240690: Race condition between EDT and BasicDirectoryModel.FilesLoader.run0().
Putting the entire method into synchronized (fileCache)
is an easy solution. Yet any other thread which accesses fileCache
will be blocked until the code exits the synchronized block in the call
method. This somewhat defeats updating the file list in the background, doesn't it?
Even if you'll go this route, I'm for replacing Vector
with ArrayList
for the newFileCache
and newFiles
variables. These are local variables, they're not accessed concurrently. Yet they're accessed from two threads: the current one and the one where ShellFolder.invoke
runs, so there could be a need to use another synchronisation technique to ensure thread-safety between these two threads.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Even if you'll go this route, I'm for replacing
Vector
withArrayList
for thenewFileCache
andnewFiles
variables. These are local variables, they're not accessed concurrently. Yet they're accessed from two threads: the current one and the one whereShellFolder.invoke
runs, so there could be a need to use another synchronisation technique to ensure thread-safety between these two threads.
Another Synchronization technique even after synchronized (fileCache)
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Even if you'll go this route, I'm for replacing
Vector
withArrayList
for thenewFileCache
andnewFiles
variables. These are local variables, they're not accessed concurrently. Yet they're accessed from two threads: the current one and the one whereShellFolder.invoke
runs, so there could be a need to use another synchronisation technique to ensure thread-safety between these two threads.Another Synchronization technique even after
synchronized (fileCache)
?
Yes, even after.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
…I'm for replacing
Vector
withArrayList
for thenewFileCache
andnewFiles
variables. These are local variables, they're not accessed concurrently. Yet they're accessed from two threads: the current one and the one whereShellFolder.invoke
runs, so there could be a need to use another synchronisation technique to ensure thread-safety between these two threads.Another Synchronization technique even after
synchronized (fileCache)
?Yes, even after.
No, it's not needed, actually. ShellFolder.invoke
executes the code directly on the same thread or passes it to COM thread on Windows. When it's passed to the COM thread, a Future
object is used, which ensures synchronisation.
I submitted JDK-8324973: Replace Vector with ArrayList in BasicDirectoryModel.FilesLoader.
In JDK-8307091, jdk/src/java.desktop/share/classes/javax/swing/plaf/basic/BasicDirectoryModel.java Line 364 in 2a799e5
Here, In JDK-8323670, the exception is thrown from jdk/src/java.desktop/share/classes/javax/swing/plaf/basic/BasicDirectoryModel.java Line 365 in c702dca
Here, we have exactly the same problem: However, as I pointed out above, the code in the Yes, putting everything into |
Instead of putting everything into jdk/src/java.desktop/share/classes/javax/swing/plaf/basic/BasicDirectoryModel.java Line 365 in c702dca
into synchronized(fileCache) . With this we can ensure that a local copy would be made before comparing two fileCache list, since we cannot/shouldn't depend on any addition/deletion from fileCache .
|
But this won't resolve the problem completely. A better solution could still be creating a local copy of There's still a problem: if Then I haven't figured it out for what the calculated diffs are used. I also see that the evaluation can be cancelled. In fact, I see more thread-safety issues in the implementation… There are some fields and variables which are accessed without proper synchronisation. I looked at the code once again after reading @mrserb's code review for JDK-8240690, it seems that it goes like this:
|
Taking the above into account, I appears that the code inside And you should revert the changes you made in JDK-8323670. This bug is the proof that fix didn't help. Ideally, we should have a test which reproduces the problem… Yet, as with all concurrency issues, writing such a test could be very hard. |
Yeah, sure then I'll revert the changes and update the changes. |
It was my mistake: |
It's not a problem, the There's a bug against it: JDK-8238169: BasicDirectoryModel getDirectories and DoChangeContents.run can deadlock. Does |
I think we should start investigating this one, probably by adding special delays/asserts into the JDK to track down on what threads the data is modified and used. |
According to my analysis above, there there are two threads. (The third thread is also possible if |
Upon further testing, I found out that In addition to this, there's According to this comment
the class contains portions of code copied from |
@mrserb I've been trying to write a regression test for this problem. Have I succeeded? Not quite… I am unable to get In the CI on macOS, the test runs in headless mode, in this case Thus, the test is not stable enough. Yet it still allows testing the fix. The test does not fail with Could you please run the test too? git fetch https://github.com/aivanov-jdk/jdk.git 8323670-BasicDirectoryModel-concurrency:8323670-BasicDirectoryModel-concurrency
git checkout 8323670-BasicDirectoryModel-concurrency
java test/jdk/javax/swing/JFileChooser/FileSystemView/BasicDirectoryModelConcurrency.java The commands above are similar to those provided in “Reviewing using Git” section in PRs on GitHub. |
src/java.desktop/share/classes/javax/swing/plaf/basic/BasicDirectoryModel.java
Outdated
Show resolved
Hide resolved
src/java.desktop/share/classes/javax/swing/plaf/basic/BasicDirectoryModel.java
Outdated
Show resolved
Hide resolved
src/java.desktop/share/classes/javax/swing/plaf/basic/BasicDirectoryModel.java
Outdated
Show resolved
Hide resolved
src/java.desktop/share/classes/javax/swing/plaf/basic/BasicDirectoryModel.java
Outdated
Show resolved
Hide resolved
src/java.desktop/share/classes/javax/swing/plaf/basic/BasicDirectoryModel.java
Outdated
Show resolved
Hide resolved
src/java.desktop/share/classes/javax/swing/plaf/basic/BasicDirectoryModel.java
Show resolved
Hide resolved
src/java.desktop/share/classes/javax/swing/plaf/basic/BasicDirectoryModel.java
Show resolved
Hide resolved
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good to me.
I cannot get ConcurrentModificationException
when running my test, BasicDirectoryModelConcurrency.java
, on a build with the fix.
@TejeshR13 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 28 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 |
I forgot to add a direct link to the test:
I reproduced the issue a few times on macOS and once on Linux, on a local host rather than in CI. |
Not able to reproduce in CI? |
The opposite. It fails in Oracle CI, yet it's still not stable enough. And the test doesn't clean up the files when it fails. So it needs additional work. |
/integrate |
Going to push as commit 70e7cdc.
Your commit was automatically rebased without conflicts. |
@TejeshR13 Pushed as commit 70e7cdc. 💡 You may see a message that your pull request was closed with unmerged commits. This can be safely ignored. |
Suggested fix JDK-8307091 also created concurrent exception intermittently (monthly once/quarterly once) on CI system. The issue was not able to be reproduced yet, hence proposing an alternative fix which uses iterators to compare the List.
CI testing shows green.
Progress
Issue
Reviewers
Reviewing
Using
git
Checkout this PR locally:
$ git fetch https://git.openjdk.org/jdk.git pull/17462/head:pull/17462
$ git checkout pull/17462
Update a local copy of the PR:
$ git checkout pull/17462
$ git pull https://git.openjdk.org/jdk.git pull/17462/head
Using Skara CLI tools
Checkout this PR locally:
$ git pr checkout 17462
View PR using the GUI difftool:
$ git pr show -t 17462
Using diff file
Download this PR as a diff file:
https://git.openjdk.org/jdk/pull/17462.diff
Webrev
Link to Webrev Comment