8307091: A few client tests intermittently throw ConcurrentModificationException #13783

TejeshR13 · 2023-05-03T16:13:53Z

Few test are throwing Concurrent Modification Exception Intermittently. Since the issue is intermittent and from the log and code review, It is found that the shared variable fileCache is used in comparison with the newFileCache where the comparison invokes the iterator method in AbstractList class. Since based on vector documentation it was evident that listIterator are fail-fast iterators and hence it would be better to make a separate local copy of vectors for comparing operations.
The same is implemented and tested in CI system which shows green.

Progress

Change must be properly reviewed (1 review required, with at least 1 Reviewer)
Change must not contain extraneous whitespace
Commit message must refer to an issue

Issue

JDK-8307091: A few client tests intermittently throw ConcurrentModificationException

Reviewers

Phil Race (@prrace - Reviewer) ⚠️ Review applies to 5829aade
Damon Nguyen (@DamonGuy - Committer) ⚠️ Review applies to 5829aade

Reviewing

Using git

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

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

Using Skara CLI tools

Checkout this PR locally:
$ git pr checkout 13783

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

Using diff file

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

Webrev

Link to Webrev Comment

bridgekeeper · 2023-05-03T16:15:18Z

👋 Welcome back tr! 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 · 2023-05-03T16:17:27Z

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

mlbridge · 2023-05-03T16:21:16Z

Webrevs

01: Full - Incremental (03493ba6)
00: Full (5829aade)

prrace

Looks reasonable. Fix the formatting I noted

prrace · 2023-05-03T19:59:15Z

src/java.desktop/share/classes/javax/swing/plaf/basic/BasicDirectoryModel.java

+
+                        if (start >= 0 && end > start) {
+                            List<File> listEnd_OldSize = new Vector<>(fileCache.subList(end, oldSize));
+                            if(listEnd_OldSize.equals(newFileCache.subList(start, newSize))) {


if( -> "if ("

prrace · 2023-05-03T20:04:10Z

PS update the PR summary to match the revised JBS one -
"A few client tests intermittently throw ConcurrentModificationException"

DamonGuy · 2023-05-04T02:13:27Z

Looks like there's a title mismatch blocking you from integrating but changes LGTM.
EDIT: Just noticed Phil mentioned this already

openjdk · 2023-05-04T04:49:01Z

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

8307091: A few client tests intermittently throw ConcurrentModificationException

Reviewed-by: prr, dnguyen

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 674 new commits pushed to the master branch:

caee1be: 8307305: Update debugger tests to support JTREG_TEST_THREAD_FACTORY mode
3599448: 8307147: [x86] Dangling pointer warning for Assembler::_attributes
64ac9a0: 8306946: jdk/test/lib/process/ProcessToolsStartProcessTest.java fails with "wrong number of lines in OutputAnalyzer output"
03030d4: 8307134: Add GTS root CAs
63cd0a3: 4200096: OffScreenImageSource.removeConsumer NullPointerException
db8b3cd: 8305963: Typo in java.security.Security.getProperty
dcb2f3f: 8306320: BufferedImage spec needs clarification w.r.t its implementation of the WritableRenderedImage interface
1487477: 8305815: Update Libpng to 1.6.39
705ad7d: 8306014: Update javax.net.ssl TLS tests to use SSLContextTemplate or SSLEngineTemplate
3930709: 8068925: Add @OverRide in javax.tools classes
... and 664 more: https://git.openjdk.org/jdk/compare/431e702b676e2c02224d60181c34b5fe97873d8b...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.

TejeshR13 · 2023-05-04T04:50:48Z

/integrate

openjdk · 2023-05-04T04:52:08Z

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

caee1be: 8307305: Update debugger tests to support JTREG_TEST_THREAD_FACTORY mode
3599448: 8307147: [x86] Dangling pointer warning for Assembler::_attributes
64ac9a0: 8306946: jdk/test/lib/process/ProcessToolsStartProcessTest.java fails with "wrong number of lines in OutputAnalyzer output"
03030d4: 8307134: Add GTS root CAs
63cd0a3: 4200096: OffScreenImageSource.removeConsumer NullPointerException
db8b3cd: 8305963: Typo in java.security.Security.getProperty
dcb2f3f: 8306320: BufferedImage spec needs clarification w.r.t its implementation of the WritableRenderedImage interface
1487477: 8305815: Update Libpng to 1.6.39
705ad7d: 8306014: Update javax.net.ssl TLS tests to use SSLContextTemplate or SSLEngineTemplate
3930709: 8068925: Add @OverRide in javax.tools classes
... and 664 more: https://git.openjdk.org/jdk/compare/431e702b676e2c02224d60181c34b5fe97873d8b...master

Your commit was automatically rebased without conflicts.

openjdk · 2023-05-04T04:52:18Z

@TejeshR13 Pushed as commit 465bdd9.

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

turbanoff · 2023-05-04T18:20:58Z

src/java.desktop/share/classes/javax/swing/plaf/basic/BasicDirectoryModel.java

-                                return null;
+
+                        if (start >= 0 && end > start) {
+                            List<File> listStart_OldSize = new Vector<>(fileCache.subList(start, oldSize));


Why Vector was used instead of ArrayList ?

Nothing specific reason why I choose vector apart from the main object is of Vector type and since in this context (where both the objects are not shared) both Vector/Arraylist comparison remains same. Does Arraylist comparison has any advantage here?

I already looked at this and Vector is what this class uses already and there's no reason to be different and definitely no reason to change the rest.

Fix

5829aad

openjdk bot added the rfr Pull request is ready for review label May 3, 2023

openjdk bot added the client client-libs-dev@openjdk.org label May 3, 2023

prrace approved these changes May 3, 2023

View reviewed changes

DamonGuy approved these changes May 4, 2023

View reviewed changes

Updated based on review comments

03493ba

TejeshR13 changed the title ~~8307091: Few test threw ConcurrentModificationException Intermittently~~ 8307091: A few client tests intermittently throw ConcurrentModificationException May 4, 2023

openjdk bot added the ready Pull request is ready to be integrated label May 4, 2023

openjdk bot added the integrated Pull request has been integrated label May 4, 2023

openjdk bot closed this May 4, 2023

openjdk bot removed ready Pull request is ready to be integrated rfr Pull request is ready for review labels May 4, 2023

turbanoff reviewed May 4, 2023

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

8307091: A few client tests intermittently throw ConcurrentModificationException #13783

8307091: A few client tests intermittently throw ConcurrentModificationException #13783

TejeshR13 commented May 3, 2023 •

edited by openjdk bot

Loading

bridgekeeper bot commented May 3, 2023

openjdk bot commented May 3, 2023

mlbridge bot commented May 3, 2023 •

edited

Loading

prrace left a comment

prrace May 3, 2023

TejeshR13 May 4, 2023

prrace commented May 3, 2023

DamonGuy commented May 4, 2023 •

edited

Loading

openjdk bot commented May 4, 2023

TejeshR13 commented May 4, 2023

openjdk bot commented May 4, 2023

openjdk bot commented May 4, 2023

turbanoff May 4, 2023

TejeshR13 May 5, 2023

prrace May 5, 2023

8307091: A few client tests intermittently throw ConcurrentModificationException #13783

8307091: A few client tests intermittently throw ConcurrentModificationException #13783

Conversation

TejeshR13 commented May 3, 2023 • edited by openjdk bot Loading

Progress

Issue

Reviewers

Reviewing

Webrev

bridgekeeper bot commented May 3, 2023

openjdk bot commented May 3, 2023

mlbridge bot commented May 3, 2023 • edited Loading

Webrevs

prrace left a comment

Choose a reason for hiding this comment

prrace May 3, 2023

Choose a reason for hiding this comment

TejeshR13 May 4, 2023

Choose a reason for hiding this comment

prrace commented May 3, 2023

DamonGuy commented May 4, 2023 • edited Loading

openjdk bot commented May 4, 2023

TejeshR13 commented May 4, 2023

openjdk bot commented May 4, 2023

openjdk bot commented May 4, 2023

turbanoff May 4, 2023

Choose a reason for hiding this comment

TejeshR13 May 5, 2023

Choose a reason for hiding this comment

prrace May 5, 2023

Choose a reason for hiding this comment

TejeshR13 commented May 3, 2023 •

edited by openjdk bot

Loading

mlbridge bot commented May 3, 2023 •

edited

Loading

DamonGuy commented May 4, 2023 •

edited

Loading