Skip to content
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

8283715: Update ObjectStreamClass to be final #8009

Conversation

stuart-marks
Copy link
Member

@stuart-marks stuart-marks commented Mar 29, 2022

Pretty much just what it says.


Progress

  • Change must not contain extraneous whitespace
  • Commit message must refer to an issue
  • Change must be properly reviewed
  • Change requires a CSR request to be approved

Issues

Reviewers

Reviewing

Using git

Checkout this PR locally:
$ git fetch https://git.openjdk.java.net/jdk pull/8009/head:pull/8009
$ git checkout pull/8009

Update a local copy of the PR:
$ git checkout pull/8009
$ git pull https://git.openjdk.java.net/jdk pull/8009/head

Using Skara CLI tools

Checkout this PR locally:
$ git pr checkout 8009

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

Using diff file

Download this PR as a diff file:
https://git.openjdk.java.net/jdk/pull/8009.diff

@bridgekeeper
Copy link

bridgekeeper bot commented Mar 29, 2022

👋 Welcome back smarks! 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
Copy link

openjdk bot commented Mar 29, 2022

@stuart-marks 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 openjdk bot added the core-libs label Mar 29, 2022
@stuart-marks
Copy link
Member Author

stuart-marks commented Mar 29, 2022

/csr

@openjdk openjdk bot added the csr label Mar 29, 2022
@openjdk
Copy link

openjdk bot commented Mar 29, 2022

@stuart-marks has indicated that a compatibility and specification (CSR) request is needed for this pull request.
@stuart-marks this pull request must refer to an issue in JBS to be able to link it to a CSR request. To refer this pull request to an issue in JBS, please use the /issue command in a comment in this pull request.

@stuart-marks
Copy link
Member Author

stuart-marks commented Mar 29, 2022

/issue JDK-8283715

@openjdk openjdk bot changed the title Make ObjectStreamClass final. 8283715: Update ObjectStreamClass to be final Mar 29, 2022
@openjdk
Copy link

openjdk bot commented Mar 29, 2022

@stuart-marks 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.

@openjdk openjdk bot added the rfr label Mar 29, 2022
@stuart-marks
Copy link
Member Author

stuart-marks commented Mar 29, 2022

Please review CSR request JDK-8283811.

@mlbridge
Copy link

mlbridge bot commented Mar 29, 2022

Webrevs

@jaikiran
Copy link
Member

jaikiran commented Mar 29, 2022

The test failures caught by GitHub Actions jobs look related to the area of this change:

2022-03-29T02:21:21.2187570Z 	at CheckCSMs.main(CheckCSMs.java:98)
2022-03-29T02:21:21.2188140Z 	at java.base/jdk.internal.reflect.DirectMethodHandleAccessor.invoke(DirectMethodHandleAccessor.java:104)
2022-03-29T02:21:21.2188650Z 	at java.base/java.lang.reflect.Method.invoke(Method.java:577)
2022-03-29T02:21:21.2189060Z 	at com.sun.javatest.regtest.agent.MainWrapper$MainThread.run(MainWrapper.java:127)
2022-03-29T02:21:21.2189420Z 	at java.base/java.lang.Thread.run(Thread.java:828)
2022-03-29T02:21:21.2189590Z 
2022-03-29T02:21:21.2190030Z JavaTest Message: Test threw exception: java.lang.RuntimeException: Unexpected non-final instance method: 
2022-03-29T02:21:21.2190400Z java/io/ObjectStreamField#getType ()Ljava/lang/Class;

@dfuch
Copy link
Member

dfuch commented Mar 29, 2022

Yes - it looks like jdk/internal/reflect/CallerSensitive/CheckCSMs.java needs to be updated.

@RogerRiggs
Copy link
Contributor

RogerRiggs commented Mar 29, 2022

Close by is java.io.ObjectStreamField which seems also useful to make final.
Though it does have a public constructor so is more likely to raise a compatibility concern.

@stuart-marks
Copy link
Member Author

stuart-marks commented Mar 29, 2022

Yes, the CheckCSMs test needed to be updated to remove ObjectStreamClass#forClass from the list of non-final CSMs, as that method is now part of a final class. The error message about ObjectStreamField#getType is misleading; I've fixed up the error reporting for this case.

@stuart-marks
Copy link
Member Author

stuart-marks commented Mar 29, 2022

It would indeed be interesting to make adjustments to ObjectStreamField. However, it's publicly subclassable, and it's not serializable, so a different set of dynamics apply. I'd like to consider that effort separately from this PR.

@mlchung
Copy link
Member

mlchung commented Mar 29, 2022

Looks good. Thanks for improving the error reporting in CheckCSMs test.

dfuch
dfuch approved these changes Mar 29, 2022
Copy link
Member

@dfuch dfuch left a comment

Nice update to the test!

@jaikiran
Copy link
Member

jaikiran commented Mar 30, 2022

Hello Stuart, the test change looks fine to me. Just a minor note - the copyright year of the test will need an update.

@openjdk openjdk bot removed the csr label Mar 30, 2022
@openjdk
Copy link

openjdk bot commented Mar 30, 2022

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

8283715: Update ObjectStreamClass to be final

Reviewed-by: darcy, jpai, mchung, dfuchs

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

  • eb5b712: 8283701: Add final or sealed modifier to appropriate java.awt.color ICC_Profile API classes
  • d066856: 8282162: [vector] Optimize integral vector negation API
  • bfd9c2b: 8283015: Create a test for JDK-4715496
  • 8cdabea: 8207025: JvmtiEnv::SetSystemProperty() does not handle OOM
  • 272d653: 8283889: Fix Typo in open/src/java.sql/share/classes/java/sql/package-info.java
  • a9a9b90: 8283723: Update Visual Studio 2022 to version 17.1.0 for Oracle builds on Windows
  • 489b27d: 8282776: Bad NullPointerException message when invoking an interface MethodHandle on a null receiver
  • 072f2c4: 8283782: Redundant verification of year in LocalDate::ofEpochDay
  • 2fef5d4: 8281853: serviceability/sa/ClhsdbThreadContext.java failed with NullPointerException: Cannot invoke "sun.jvm.hotspot.gc.shared.GenCollectedHeap.getGen(int)" because "this.heap" is null
  • f9f439a: 8283717: vmTestbase/nsk/jdi/ThreadStartEvent/thread/thread001 failed due to SocketTimeoutException
  • ... and 31 more: https://git.openjdk.java.net/jdk/compare/f520b4f891b71c630bc13f5db4f305194ef227e5...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 openjdk bot added the ready label Mar 30, 2022
@stuart-marks
Copy link
Member Author

stuart-marks commented Mar 30, 2022

@jaikiran Thanks, I've updated the copyright year. I will integrate when I can keep an eye on the subsequent builds.

@stuart-marks
Copy link
Member Author

stuart-marks commented Mar 30, 2022

/integrate

@openjdk
Copy link

openjdk bot commented Mar 30, 2022

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

Your commit was automatically rebased without conflicts.

@openjdk openjdk bot added the integrated label Mar 30, 2022
@openjdk openjdk bot closed this Mar 30, 2022
@openjdk openjdk bot removed ready rfr labels Mar 30, 2022
@openjdk
Copy link

openjdk bot commented Mar 30, 2022

@stuart-marks Pushed as commit ae57258.

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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
core-libs integrated
6 participants