-
Notifications
You must be signed in to change notification settings - Fork 6.2k
8355022: Implement JEP 506: Scoped Values #24923
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
Conversation
|
👋 Welcome back aph! A progress list of the required criteria for merging this PR into |
|
@theRealAph 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 16 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 |
|
@theRealAph The following labels 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 lists. If you would like to change these labels, use the /label pull request command. |
|
Nit: is the no-arg constructor of |
|
@theRealAph There are test updates in the loom repo that will also need to be included in this PR. test/jdk/java/lang/ScopedValue/ScopedValueAPI.java at least. |
Webrevs
|
| * @throws StructureViolationException if a structure violation is detected | ||
| * @throws X if {@code op} completes with an exception | ||
| * @since 23 | ||
| * @since 25 |
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.
The @since on the method be dropped once Carrier is bumped to 25.
|
@theRealAph Can you include the update to javax/security/auth/Subject.java as part of this? |
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 occurred to me that ScopedValue.NEW_THREAD_BINDINGS can be made package‑private and used in Thread, removing duplication in Thread.NEW_THREAD_BINDINGS.
This was done this way back when ScopedValue lived under the jdk.incubator.* package tree.
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 think it would be better not change that in this PR, as this PR is about making the feature permanent.
|
@theRealAph this pull request can not be integrated into git checkout JDK-8355022
git fetch https://git.openjdk.org/jdk.git master
git merge FETCH_HEAD
# resolve conflicts and follow the instructions given by git merge
git commit -m "Merge master"
git push |
Fixed now. |
| * @return the value of the scoped value if bound, otherwise {@code other} | ||
| */ | ||
| public T orElse(T other) { | ||
| Objects.requireNonNull(other); |
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.
Shouldn't the NPE be specified in the Javadoc?
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.
In the class specification, right above the API notes:
* <p> Unless otherwise specified, passing a {@code null} argument to a method in this
* class will cause a {@link NullPointerException} to be thrown.
|
I noted that Context: test/jdk/jdk/internal/misc/ThreadFlock/WithScopedValue.java |
liach
left a comment
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.
Reviewed the Javadoc and specification updates and @enablePreview removals for the end of preview. These are all related tests that can have the directive removed, aside from the thread flock test I asked about.
|
/reviewers 2 reviewer |
ThreadFlock supports the inheritance of scoped values into structured concurrency contexts. No changes in this PR to that construct. |
|
@theRealAph One other test that will need attention is runtime/ClassFile/ClassFileVersionTest.java. |
|
May I provide a quick patch to use an alternative mechanism to test preview class file versions for that class instead of changing that in this PR? I have created #25128 addressing this issue. |
That would be good as it shouldn't depend on ScopedValue being a preview API class. |
That's an interesting question that I'll leave to @AlanBateman . |
Chen offered to change this test to use the ClassFile API and construct the class with the preview minor number. The PR to change this test is reviewed so I think all good, nothing more to do here. |
Nothing to do as the test uses StructureViolationException so needs to continue to be compile/run with preview features enabled. |
|
I ran tier1-6 so I think we are good and there isn't anything left to bring over from the loom repo. There are a few files that will need their copyright header updated before integration. The CSR also needs to be finalized. |
liach
left a comment
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.
The latest master merge looks good.
AlanBateman
left a comment
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.
CSR is approved, I think this update is ready to integrate.
|
/integrate |
|
Going to push as commit 4e1878c.
Your commit was automatically rebased without conflicts. |
|
@theRealAph Pushed as commit 4e1878c. 💡 You may see a message that your pull request was closed with unmerged commits. This can be safely ignored. |
Propose to finalize scoped values.
The only functional change is that the orElse() method no longer accepts a null argument.
Progress
Issues
Reviewers
Reviewing
Using
gitCheckout this PR locally:
$ git fetch https://git.openjdk.org/jdk.git pull/24923/head:pull/24923$ git checkout pull/24923Update a local copy of the PR:
$ git checkout pull/24923$ git pull https://git.openjdk.org/jdk.git pull/24923/headUsing Skara CLI tools
Checkout this PR locally:
$ git pr checkout 24923View PR using the GUI difftool:
$ git pr show -t 24923Using diff file
Download this PR as a diff file:
https://git.openjdk.org/jdk/pull/24923.diff
Using Webrev
Link to Webrev Comment