8340572: ConcurrentModificationException when sorting ArrayList sublists

[szegedi]

Fixes a regression with #17818 where ArrayList.subList(…).sort() started incrementing ArrayList.modCount resulting in some cases throwing a ConcurrentModificationException where none was thrown before.

This change keeps the optimization from #17818 but restores the behavior where only sorting the ArrayList changes the mod count, but sorting its sublists does not.

---

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-8340572: ConcurrentModificationException when sorting ArrayList sublists (Bug - P2)

Reviewers

• Stuart Marks (@stuart-marks - Reviewer)

Reviewing

Using git

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

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

Using Skara CLI tools

Checkout this PR locally:
$ git pr checkout 21250

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

Using diff file

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

Webrev

Link to Webrev Comment

[bridgekeeper]

👋 Welcome back attila! 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]

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

8340572: ConcurrentModificationException when sorting ArrayList sublists

Reviewed-by: smarks

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

• 86e3d52: 8341258: Open source few various AWT tests - Set1
• 92cb633: 8340967: Open source few Cursor tests - Set2
• 3d38cd9: 8340966: Open source few Checkbox and Cursor tests - Set1
• 7e3978e: 8340164: Open source few Component tests - Set1
• c8e70df: 8341162: Open source some of the AWT window test
• 2e5b420: 8340326: Remove references to Applet in core-libs/security tests
• f5f0852: 8341379: Shenandoah: Improve lock contention during cleanup
• 04c9c5f: 8341111: open source several AWT tests including menu shortcut tests
• 0dd4997: 8340555: Open source DnD tests - Set4
• beb2a51: 8341377: Update VMProps.isCDSRuntimeOptionsCompatible to include Parallel and Serial GC
• ... and 93 more: https://git.openjdk.org/jdk/compare/73ebb848fdb66861e912ea747c039ddd1f7a5f48...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]

@szegedi The following label will be automatically applied to this pull request:

• core-libs

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]

@szegedi Please do not rebase or force-push to an active PR as it invalidates existing review comments. Note for future reference, the bots always squash all changes into a single commit automatically as part of the integration. See OpenJDK Developers’ Guide for more information.

[mlbridge]

Webrevs

• 01: Full - Incremental (e0ae1cc9)
• 00: Full (43185d7e)

[dholmes-ora]

Just an observation, but sorting is not defined as a "structural modification" but obviously would interfere with an active iterator. So the docs may need updating to include this aspect.

[dfuch]

I am not a spcialist here. But if sorting the parent list is considered as a modification, and if the sublist is just a view of the parent list, then surely sorting/modifying the sublist should be considered as a modification of the parent list?

In which case the issue here is probably deeper: SubList and parent list appear to maintain separate modcount variables which are only loosely coupled. Maybe that's the bug that ought to be fixed?

[szegedi]

@dholmes-ora and @dfuch both your observations are quite valid and I agree with them. If you look at the JBS issue, we discussed this topic there.

Clarifying the collections' behavior with regard to when to throw a CME, and hopefully making that behavior be consistent would be a welcome enhancement. As things stand, the scope of this fix is just ensuring that the behavior of ArrayList reverts back to what it was prior to my optimization, since the change triggered a test failure in some Google test suite. All the while recognizing that the behavior was not particularly consistent to begin with.

[dholmes-ora]

I agree with just reverting the behaviour as discussed in JBS, but will leave the actual review for the libs folk. The more general question of when to trigger CME is a very thorny one.

[stuart-marks]

Right, @dholmes-ora, you might recall that we argued about this several years ago. :-) Even though I'm on the losing end of that argument, I am ok with eventually removing the modCount increments from sort and replaceAll. This is covered by JDK-8203704 which should include a spec clarification to support the change in behavior. Meanwhile, this PR should simply revert to the behavior previous to the JDK-8325679 change. (Even though that behavior is arguably wrong.)

[stuart-marks]

@szegedi Thanks for starting this. Just a couple comments on the test.

1. It would be good to have a test case that verifies that sorting the entire list does increment modcount, in addition to the existing test case that tests that sorting a sublist does not increment modcount. (As before, this is arguably not the correct behavior, but it's the behavior that the system exhibited prior to JDK-8325679.)
2. It would be good for the test to have a more descriptive name than just the bug ID. Something like SortingModCount or similar would be fine.

Thanks.

[szegedi]

@stuart-marks and @liach thanks, those are good suggestions – done.

[stuart-marks]

Thanks for the updates! I've run these changes through our internal testing system and the results look good.

[szegedi]

/integrate

[openjdk]

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

• 9a25f82: 8339386: Assertion on AIX - original PC must be in the main code section of the compiled method
• df763cd: 8341558: [AIX] build broken after 8341413
• 1c3e56c: 8341512: Optimize StackMapGenerator::processInvokeInstructions
• f8db3a8: 8341510: Optimize StackMapGenerator::processFieldInstructions
• b42fbf4: 8339699: Optimize DataOutputStream writeUTF
• 5592894: 8340417: Open source some MenuBar tests - Set1
• bade041: 8341554: Shenandoah: Missing heap lock when updating usage for soft ref policy
• 85e0e64: 8341443: [macos] AppContentTest and SigningOptionsTest failed due to "codesign" does not fails with "--app-content" on macOS 15
• 33e4bfd: 8341295: Add some useful debugging APIs to the debug agent
• a3e2357: 8341483: Clarify special case handling of Types.getArrayType
• ... and 104 more: https://git.openjdk.org/jdk/compare/73ebb848fdb66861e912ea747c039ddd1f7a5f48...master

Your commit was automatically rebased without conflicts.

[openjdk]

@szegedi Pushed as commit 260d465.

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