8323659: LinkedTransferQueue add and put methods call overridable offer

[ChrisHegarty]

Update LinkedTransferQueue add and put methods to not call overridable offer.

---

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-8323659: LinkedTransferQueue add and put methods call overridable offer (Bug - P2)

Reviewers

• Alan Bateman (@AlanBateman - Reviewer)

Reviewing

Using git

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

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

Using Skara CLI tools

Checkout this PR locally:
$ git pr checkout 17393

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

Using diff file

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

Webrev

Link to Webrev Comment

[bridgekeeper]

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

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

[ChrisHegarty]

/issue add JDK-8323659

[openjdk]

@ChrisHegarty The primary solved issue for a PR is set through the PR title. Since the current title does not contain an issue reference, it will now be updated.

[mlbridge]

Webrevs

• 06: Full - Incremental (ddaab989)
• 05: Full - Incremental (3aa026fa)
• 04: Full - Incremental (72ad71fb)
• 03: Full - Incremental (22f2857c)
• 02: Full - Incremental (1097a6bc)
• 01: Full - Incremental (b11804d1)
• 00: Full (206e2652)

[AlanBateman]

This feels more like a hazard when extending to override behavior. I think it would be useful to provide a summary on what the override wants to do, maybe there are better ways to customise by wrapping the LTQ rather than subclassing.

[pavelrappo]

A process-related comment: this PR is against mainline, but the issue's Fix Version/s is 22. This will create a bit of a mess if integrated.

[ChrisHegarty]

This feels more like a hazard when extending to override behavior.

Yeah, this is certainly a potential issue with any subclass-able class. I remember seeing similar before in other areas too.

I think it would be useful to provide a summary on what the override wants to do, maybe there are better ways to customise by wrapping the LTQ rather than subclassing.

Yeah, and we can likely refactor our code to workaround this change in JDK 22. I added a summary and reproducer in the JIRA, which should help. I'll try to explain a little here. The crux of what the code is attempting to do is to scale the executor to max pool size by refusing tasks if there the pool is less than the max, then later intercepting rejections and putting the task on the queue.

[ChrisHegarty]

A process-related comment: this PR is against mainline, but the issue's Fix Version/s is 22. This will create a bit of a mess if integrated.

Thanks for the reminder Pavel. If accepted, then the change will be applicable to 23, 22, and 21. 0.2. I'll fix up and retarget the appropriate branches, repos, once we agree a way forward.

[ChrisHegarty]

this PR is against mainline, but the issue's Fix Version/s is 22.

I updated the fix version in JIRA, and followed the process as outlined in https://mail.openjdk.org/pipermail/jdk-dev/2023-December/008560.html

[viktorklang-ora]

@ChrisHegarty My 2¢—this change could impact existing classes which have extended LTQ and knowingly, or unknowingly, presume that add and put delegate to offer, which would (silently) break those classes when running on a newer JDK (presuming this change gets merged).

I think the change is in principle correct, with that said, I want to make sure that the change is worth the potential compatibility risk (if any).

[ChrisHegarty]

@ChrisHegarty My 2¢—this change could impact existing classes which have extended LTQ and knowingly, or unknowingly, presume that add and put delegate to offer, which would (silently) break those classes when running on a newer JDK (presuming this change gets merged).

I think the change is in principle correct, with that said, I want to make sure that the change is worth the potential compatibility risk (if any).

Hi @viktorklang-ora , I agree with your comment regarding the potential impact, and the assumption of the implementation, however, you got it the wrong way round! ;-) The change I am proposing restores previous behaviour to pre-JDK 22. It is JDK 22 that has changed this. This is exactly why I am proposing the change

[viktorklang-ora]

@ChrisHegarty 😅 Well then I agree

[ChrisHegarty]

FTR - I agree that it's kinda annoying to be proposing this change and it is true that the consuming user code is making an assumption, but the impact of this is significant - leads to apparently vanishing tasks when Elasticsearch runs on JDK 22 EA. The proposed changes are extremely minimal.

If @DougLea agrees, then I'll add a minimal test case and get the PR into a clean state.

[ChrisHegarty]

Additionally, we absolutely need to fix this in JDK 21.0.2 - since a patch release should not change behaviour (from 21.0.1), in this way.

[dholmes-ora]

With my CSR hat on, JDK-8301341 should never have made the changes it did without going through a CSR request. We have been bitten by this kind of problem many times. Unless a public method is specified to utilise another public method, we should never implement one public method in terms of another (non-final one) due to the overriding problem. Backporting such a change to 21u is then potentially very damaging.

[AlanBateman]

With my CSR hat on, JDK-8301341 should never have made the changes it did without going through a CSR request. We have been bitten by this kind of problem many times. Unless a public method is specified to utilise another public method, we should never implement one public method in terms of another (non-final one) due to the overriding problem.

JDK-8301341 was a big update, a lot of refactoring to hollow out SQ and it was just an oversight that we didn't spot the methods now use an overridable method. It's something to always look out for in the collections area, just missed here.

[ChrisHegarty]

It's unfortunate that we're only discovering this now, since 21.0.2 is scheduled to release tomorrow, Jan 16th, and we've not yet gotten this reviewed and merged into master or jdk22 yet. We can decided how to proceed with the backports once this PR has agreed the approach and is reviewed.

[DougLea]

Sorry for needlessly calling overridable versions in these two cases. I should have caught that.

[ChrisHegarty]

Sorry for needlessly calling overridable versions in these two cases. I should have caught that.

No problem, easy to overlook. I assume you are ok with the changes? If so, could I please ask you to add your review. Otherwise, is there another way that we should proceed?

[openjdk]

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

8323659: LinkedTransferQueue add and put methods call overridable offer

Reviewed-by: alanb

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 no new commits pushed to the master branch. If another commit should be pushed before you perform the /integrate command, your PR will be automatically rebased. If you prefer to avoid any potential 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.

[DougLea]

Yes, the change looks right to me -- changing the only two calls that could matter here.

[ChrisHegarty]

Thanks for the reviews @AlanBateman and @DougLea

[ChrisHegarty]

/integrate

[openjdk]

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

• 5045839: 8323635: Test gc/g1/TestHumongousAllocConcurrentStart.java fails with -XX:TieredStopAtLevel=3
• 44a9392: 8323780: Serial: Remove unused _full_collections_completed
• 5906240: 8323716: Only print ZGC Phase Switch events in hs_err files when running with ZGC
• e01f6da: 8320175: [BACKOUT] 8316533: C2 compilation fails with assert(verify(phase)) failed: missing Value() optimization

Your commit was automatically rebased without conflicts.

[openjdk]

@ChrisHegarty Pushed as commit ee4d9aa.

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

[jddarcy]

With my CSR hat on, JDK-8301341 should never have made the changes it did without going through a CSR request. We have been bitten by this kind of problem many times. Unless a public method is specified to utilise another public method, we should never implement one public method in terms of another (non-final one) due to the overriding problem.

JDK-8301341 was a big update, a lot of refactoring to hollow out SQ and it was just an oversight that we didn't spot the methods now use an overridable method. It's something to always look out for in the collections area, just missed here.

We can and have run retroactive CSRs in cases like this before; I recommend we do one now.

[AlanBateman]

We can and have run retroactive CSRs in cases like this before; I recommend we do one now.

Yes although the issue will be mute once JDK-8323659 is integrated into jdk22.

[ChrisHegarty]

I just integrated the fix into jdk 22, so we’re good there now.

The final piece of the puzzle is jdk 21.0.2, which we’re too late to fix. We can add a release note, and fix it in 21.0.3. Any objections or alternative suggestions?