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

8259522: Apply java.io.Serial annotations in java.desktop #2020

Closed
wants to merge 9 commits into from

Conversation

@mrserb
Copy link
Member

@mrserb mrserb commented Jan 11, 2021

Please review the application of @java.io.Serial annotation (JDK-8202385) to types in the desktop module to enable stricter compile-time checking of serialization-related declarations.

This annotation can be applied to these methods in the module:

private void writeObject(java.io.ObjectOutputStream stream) throws IOException
private void readObject(java.io.ObjectInputStream stream) throws IOException, ClassNotFoundException
private void readObjectNoData() throws ObjectStreamException
ANY-ACCESS-MODIFIER Object writeReplace() throws ObjectStreamException
ANY-ACCESS-MODIFIER Object readResolve() throws ObjectStreamException
private static final ObjectStreamField[] serialPersistentFields
private static final long serialVersionUID

Notes:

  • I have tried to update the comments for serialVersionUID as accurately as possible, but mostly based on the source code history and bugs in JBS where that field was added
  • Some of the readObject/writeObject methods in the javax.swing package does not have a spec, because this package and some others are excluded from the serialization specification.

A similar fix was implemented for java.base module as well:
http://mail.openjdk.java.net/pipermail/core-libs-dev/2019-August/062046.html


Progress

  • Change must not contain extraneous whitespace
  • Commit message must refer to an issue
  • Change must be properly reviewed

Issue

  • JDK-8259522: Apply java.io.Serial annotations in java.desktop

Reviewers

Download

$ git fetch https://git.openjdk.java.net/jdk pull/2020/head:pull/2020
$ git checkout pull/2020

@bridgekeeper
Copy link

@bridgekeeper bridgekeeper bot commented Jan 11, 2021

👋 Welcome back serb! 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.

Loading

@openjdk
Copy link

@openjdk openjdk bot commented Jan 11, 2021

@mrserb The following labels will be automatically applied to this pull request:

  • 2d
  • awt
  • beans
  • i18n
  • sound
  • swing

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.

Loading

@mrserb mrserb marked this pull request as ready for review Jan 11, 2021
@openjdk openjdk bot added the rfr label Jan 11, 2021
@mlbridge
Copy link

@mlbridge mlbridge bot commented Jan 11, 2021

Webrevs

Loading

/**
* Use serialVersionUID from JDK 1.7 for interoperability.
*/
@Serial
private static final long serialVersionUID = 1L;
Copy link
Member

@aivanov-jdk aivanov-jdk Jan 12, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is the standard wording, yet should it mention the serialization is between the same versions only because the value of serialVersionUID is not unique?

Loading

Copy link
Member Author

@mrserb mrserb Jan 12, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think it is "quite" unique, the "1L" is used from the beginning. It is just different from the value which could be generated by the "serialver" but still should work fine.

Loading

Copy link
Member

@aivanov-jdk aivanov-jdk Jan 12, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Okay. There are several classes which extend this one, all of them use the same 1L value which makes it not "quite" unique.

Changing the values at this time presents more risks than benefits.

Loading

Loading
Loading
@openjdk
Copy link

@openjdk openjdk bot commented Jan 12, 2021

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

8259522: Apply java.io.Serial annotations in java.desktop

Reviewed-by: aivanov, psadhukhan

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

  • a483869: 8225045: javax/swing/JInternalFrame/8146321/JInternalFrameIconTest.java fails on linux-x64
  • 44c8379: 8256019: JLabel HTML text does not support translucent text colors
  • 0957d9e: 8259519: The java.awt.datatransfer.DataFlavor#ioInputStreamClass field is redundant
  • 65bed64: 8253635: Implement toString() for SSLEngineImpl
  • c6d798c: 8259629: aarch64 builds fail after JDK-8258932
  • 4be2173: 8259319: Illegal package access when SunPKCS11 requires SunJCE's classes
  • e4df209: 7018932: Drawing very large coordinates with a dashed Stroke can cause Java to hang
  • 5f7ccce: 8226810: Failed to launch JVM because of NullPointerException occured on System.props
  • d6a2105: 8259343: [macOS] Update JNI error handling in Cocoa code.
  • c338f11: 8259349: -XX:AvgMonitorsPerThreadEstimate=1 does not work right
  • ... and 51 more: https://git.openjdk.java.net/jdk/compare/81db63e8d74c0d85cdadb0a70580a33c79d0bb80...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.

Loading

@openjdk openjdk bot added the ready label Jan 12, 2021
mrserb and others added 2 commits Jan 12, 2021
…ption.java

Co-authored-by: Aleksei Ivanov <70774172+aivanov-jdk@users.noreply.github.com>
…on.java

Co-authored-by: Aleksei Ivanov <70774172+aivanov-jdk@users.noreply.github.com>
Copy link
Contributor

@prsadhuk prsadhuk left a comment

Why do we add serialVersionUID in some classes like DefaultMutableTreeNode.java but not in other swing classes?
Also, if this change is for stricter compile-time checking, shouldn't we remove @SuppressWarnings("serial") check?

Loading

@mrserb
Copy link
Member Author

@mrserb mrserb commented Jan 13, 2021

Why do we add serialVersionUID in some classes like DefaultMutableTreeNode.java but not in other swing classes?

Most Swing classes are marked by the specific "Warning" that "Same-version serialization only" is supported. (I think such a warning is missed in a few classes). So generally the serialVersionUID field is not needed in such classes, but if present this provides a small benefit -> this UID is not generated at runtime.

For example, the DefaultMutableTreeNode was updated by the JDK-5017904 fix, which was unrelated to serialization but was targeted for the performance issue.

Also, if this change is for stricter compile-time checking, shouldn't we remove @SuppressWarnings("serial") check?

At some point, we probably can remove it but will need to fix all serialization warnings which were disabled.

Loading

@mrserb
Copy link
Member Author

@mrserb mrserb commented Jan 15, 2021

/integrate

Loading

@openjdk openjdk bot closed this Jan 15, 2021
@openjdk openjdk bot added integrated and removed ready labels Jan 15, 2021
@openjdk openjdk bot removed the rfr label Jan 15, 2021
@openjdk
Copy link

@openjdk openjdk bot commented Jan 15, 2021

@mrserb Since your change was applied there have been 111 commits pushed to the master branch:

  • bf28f92: 8259713: Fix comments about ResetNoHandleMark in deoptimization
  • 4f881ba: 8258652: Assert in JvmtiThreadState::cur_stack_depth() can noticeably slow down debugging single stepping
  • d18d26e: 8259350: Add some internal debugging APIs to the debug agent
  • a6b2162: 8259278: Optimize Vector API slice and unslice operations
  • da6bcf9: 8255019: Shenandoah: Split STW and concurrent mark into separate classes
  • aba3431: 8258956: Memory Leak in StringCoding on ThreadLocal resultCached StringCoding.Result
  • 8554fe6: 8253866: Security Libs Terminology Refresh
  • c2a3c7e: 8259727: Remove redundant "target" arguments to methods in Links
  • 1620664: 8259723: Move Table class to formats.html package
  • 38a1201: 8258912: Remove JVM options CountJNICalls and CountJVMCalls
  • ... and 101 more: https://git.openjdk.java.net/jdk/compare/81db63e8d74c0d85cdadb0a70580a33c79d0bb80...master

Your commit was automatically rebased without conflicts.

Pushed as commit 978bed6.

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

Loading

@mrserb mrserb deleted the JDK-8259522 branch Apr 12, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
3 participants