8336843: Deprecate java.util.zip.ZipError for removal #20642

eirbjo · 2024-08-20T10:24:25Z

Please review this PR which suggests to deprecate the unused class java.util.zip.ZipError for removal.

The class has been unsed by OpenJDK since the ZIP API was rewritten from native to Java in JDK 9.

I opted to not explain the reason for the deprecation in detail, but instead simply point to ZipException as an alternative. Should more explanation be desired, I could prepend that with a note saying that the class is unused since JDK 9.

A CSR for this API update has been drafted, I'll update the Specification section there once we reach a concensus on the deprecation note in this PR.

This deprecation was initially suggested here: https://mail.openjdk.org/pipermail/core-libs-dev/2024-June/125720.html

Progress

Change must not contain extraneous whitespace
Commit message must refer to an issue
Change requires CSR request JDK-8338663 to be approved
Change must be properly reviewed (2 reviews required, with at least 2 Reviewers)

Issues

JDK-8336843: Deprecate java.util.zip.ZipError for removal (Enhancement - P4)
JDK-8338663: Deprecate java.util.zip.ZipError for removal (CSR)

Reviewers

Chen Liang (@liach - Reviewer)
Lance Andersen (@LanceAndersen - Reviewer)

Reviewing

Using git

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

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

Using Skara CLI tools

Checkout this PR locally:
$ git pr checkout 20642

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

Using diff file

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

Webrev

Link to Webrev Comment

eirbjo · 2024-08-20T10:25:01Z

/csr

bridgekeeper · 2024-08-20T10:25:19Z

👋 Welcome back eirbjo! 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 · 2024-08-20T10:26:32Z

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

8336843: Deprecate java.util.zip.ZipError for removal

Reviewed-by: liach, lancea

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

de12fc7: 8339684: ResizeObserver callback interrupts smooth scrolling on Chrome
ebb4759: 8340625: Open source additional Component tests (part 3)
3ee94e0: 8341282: (fs) Move creation time fallback logic to Java layer (Linux)
f1ea57f: 8340229: Improve opening sentence of FileInputStream constructor specification
1202800: 8341006: Optimize StackMapGenerator detect frames
eb93e69: 8339979: VirtualThreadSchedulerMXBeanTest.testReduceParallelism fails intermittently
21f8ccf: 8341310: Test TestJcmdWithSideCar.java should skip ACCESS_TMP_VIA_PROC_ROOT (after JDK-8327114)
7d524d7: 8341004: Open source AWT FileDialog related tests
d7f32d8: 8341415: Optimize RawBytecodeHelper::next
6af1358: 8337753: Target class of upcall stub may be unloaded
... and 508 more: https://git.openjdk.org/jdk/compare/5671f836039ef1683e3e9ce5b7cf0fa2f1860e2d...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 · 2024-08-20T10:27:06Z

@eirbjo an approved CSR request is already required for this pull request.

openjdk · 2024-08-20T10:27:39Z

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

mlbridge · 2024-08-23T15:07:37Z

Webrevs

LanceAndersen · 2024-08-23T19:13:23Z

On a corpus search, found
org.eclipse.jdt.core/search/org/eclipse/jdt/internal/core/search/indexing/AddJarFileToIndex.java references ZipError (16 usages)

sbt/internal/inc/zip/ZipCentralDir.java referenced ZipError(was based off an old copy of ZipFS I believe), the file does not seem to be part of the GitHub repository and there are 5 usages

com/aoapps/lang/Throwables.class and com/aoindustries/lang/Throwables.class (clone of each other I believe)

com/android/tools/apk/analyzer/internal/ArchiveManagerImpl. ( 1 usage)

So would be good to also alert those projects if possible

ExE-Boss · 2024-08-23T19:59:49Z

There’s also HMCL’s org.jackhuang.hmcl.util.io.CompressingUtils, which catches ZipError to convert it to a ZipException when run on Java 8 (this was also mentioned in the linked core‑libs‑dev discussion).

LanceAndersen · 2024-08-23T20:10:22Z

There’s also HMCL’s org.jackhuang.hmcl.util.io.CompressingUtils, which catches ZipError to convert it to a ZipException when run on Java 8 (this was also mentioned in the linked core‑libs‑dev discussion).

Yes that is true. The difference there is that is due to the demo version of Zip FS and as of JDK 9, when Zip FS was supported, it did not throw ZipError

liach · 2024-08-23T20:12:53Z

A search on GitHub shows multiple Minecraft-related Java projects catch (but not create) ZipError for Java 8 backward compatibility. Maybe the actual deprecation can take steps; first with a functional removal to make ZipError constructors and methods throw errors, before finally removing it many releases later.

eirbjo · 2024-08-24T10:16:55Z

So would be good to also alert those projects if possible

I guess the deprecation warning itself is the "official" channel for communicating this type of information. But I agree it doesn't hurt to alert at least some of the larger projects, on a best-effort bases. So I have reached out to the following projects by reporting issues:

Eclipse JDT: AddJarFileToIndex.java references java.util.zip.ZipError eclipse-jdt/eclipse.jdt.core#2856
SBT / Zinc: ZipCentralDir.java references java.util.zip.ZipError sbt/zinc#1385
Android Studio apkparser: https://issuetracker.google.com/issues/361862739

src/java.base/share/classes/java/util/zip/ZipError.java

liach · 2024-08-24T14:06:08Z

src/java.base/share/classes/java/util/zip/ZipError.java

@@ -27,10 +27,11 @@

 /**
 * Signals that an unrecoverable error has occurred.
- *
+ * @deprecated Use {@link ZipException} instead.


We should probably talk about how this is no longer created by the reference implementation since JDK 9 (and JDK 8's zipfs is really a demo). Should we note that we might functionally disable the constructor first?

liach · 2024-08-24T16:20:40Z

@eirbjo I have updated the CSR to describe the solutions from both our and user sides.

I think you can update the class Javadoc to something like:

/**
 * A legacy error for invalid ZIP format from argument incorrectly regarded as an internal error.
 * This extends {@code InternalError} to maintain compatibility with preexisting client code catching
 * {@code InternalError}.
 * 
 * @deprecated
 * This error is not suitable to indicate an exception for a general ZIP file; use {@link
 * ZipException} instead.
 * <p>
 * {@code ZipError} fell out of use in the reference implementation in release 9 when the native ZIP
 * API was brought over to Java. Its last uses was in ZipFile and the demo zipfs in release 8.
 * <p>
 * Code bases supporting release 8 should update to catch InternalError instead. Old binaries can
 * use bytecode patching to upgrade CONSTANT_Class references to {@code java/util/zip/ZipError} in
 * exception handler ranges to {@code java/lang/InternalError}.
 */

I don't know if we should include such an excerpt:

/**
 * <p>
 * In a future release, implementation of {@code ZipError} will be removed before this class itself
 * is removed; at that point, {@code ZipError} can no longer be constructed or used, but can still
 * be compiled against as an aid in migration.
 */

I left the questions in CSR comment section; hope Joe Darcy can give us some advice here.

LanceAndersen · 2024-08-24T16:49:22Z

@eirbjo I have updated the CSR to describe the solutions from both our and user sides.

I think you can update the class Javadoc to something like:

/**
 * A legacy error for invalid ZIP format from argument incorrectly regarded as an internal error.
 * This extends {@code InternalError} to maintain compatibility with preexisting client code catching
 * {@code InternalError}.
 * 
 * @deprecated
 * This error is not suitable to indicate an exception for a general ZIP file; use {@link
 * ZipException} instead.
 * <p>
 * {@code ZipError} fell out of use in the reference implementation in release 9 when the native ZIP
 * API was brought over to Java. Its last uses was in ZipFile and the demo zipfs in release 8.
 * <p>
 * Code bases supporting release 8 should update to catch InternalError instead. Old binaries can
 * use bytecode patching to upgrade CONSTANT_Class references to {@code java/util/zip/ZipError} in
 * exception handler ranges to {@code java/lang/InternalError}.
 */

I don't know if we should include such an excerpt:

/**
 * <p>
 * In a future release, implementation of {@code ZipError} will be removed before this class itself
 * is removed; at that point, {@code ZipError} can no longer be constructed or used, but can still
 * be compiled against as an aid in migration.
 */

I left the questions in CSR comment section; hope Joe Darcy can give us some advice here.

While I agree an @deprecated note should be added, I do not feel it needs to be that verbose. I would simply borrow from what was done for SecurityManager. The CSR can provide some more background

I also do not see a need to anything WRT the constructors as that is overkill IMHO.

A release note will also go with this change which can suggest that code that also must support JDK 8 leverage InternalError

liach · 2024-08-24T17:22:28Z

I think linking to the CSR would be fine, but there is no prerequisite for API specs to link to CSRs. Need @jddarcy to double check here. I was emulating Thread.stop:

jdk/src/java.base/share/classes/java/lang/Thread.java

Line 1637 in 5671f83

* @deprecated This method was originally specified to "stop" a victim

LanceAndersen · 2024-08-25T13:36:18Z

I think linking to the CSR would be fine, but there is no prerequisite for API specs to link to CSRs. Need @jddarcy to double check here. I was emulating Thread.stop:

jdk/src/java.base/share/classes/java/lang/Thread.java

Line 1637 in 5671f83

* @deprecated This method was originally specified to "stop" a victim

I am not suggesting link to the CSR, what I was indicating the CSR provides more details because of the proposed removal.

Comparing to Thread::stop is not quite the same, and the verbiage you are pointing to above was added when the method was degraded to throw a UOE, not when it was deprecated for removal

The overall usage of ZipError is extremely minimal in the wild at best, from what I can tell was in a catch statement and was not documented to be thrown by any public API, though was thrown by ZipFile's internal ZipEntryIterator::next

If this was a more heavily used Exception, it would probably warrant further consideration for additional documentation, I am not convinced it is needed here

liach · 2024-08-26T15:35:13Z

How about this:

/**
 * A legacy error for invalid input ZIP format.
 *
 * @deprecated
 * Use {@link ZipException} instead. Code catching this error from the JDK 8 and earlier should update
 * to catch {@code InternalError} instead.
 */

… in JDK 9 and to mention that code may be updated to catch the super class InternalError

eirbjo · 2024-08-27T15:52:53Z

JEP-277, "Enhanced Deprecation" includes the following recommendations:

It is strongly recommended that the reasons for deprecating an API be described in that API's documentation comments. In addition, it is also recommended that potential replacement APIs be discussed and linked from the documentation.

The reason we are deprecating this class is mainly that it became obsolete in JDK 9. I think we should say just that, and not elaborate with more details of the history of the class.

The recommended API replacement depends a bit on whether the code catches or throws ZipError, but also depends on whether the code needs to support running on Java 8 and catching ZipError there. Here I think we should mention the use case of catching ZipError when running on Java 8 and briefly point in the direction of the super class InternalError.

I think I would prefer to not update the class comment of this obsolete class.

Based on the above, the PR now suggests the following documentation comment:

/**
 * Signals that an unrecoverable error has occurred.
 * @deprecated This error became obsolete in JDK 9. Use
 * {@link ZipException} instead. Code needing to catch this
 * error when running on JDK 8 or earlier releases may catch
 * the parent {@link InternalError} instead.
 */

liach

/reviewers 2 reviewer

The updated deprecation specification looks good to me. I will review the CSR when you update it. Needs Lance or another engineer in the area to confirm too.

openjdk · 2024-08-27T17:31:35Z

@liach
The total number of required reviews for this PR (including the jcheck configuration and the last /reviewers command) is now set to 2 (with at least 2 Reviewers).

src/java.base/share/classes/java/util/zip/ZipError.java

…release

LanceAndersen

I think the revised proposal looks good and once you update the CSR, let me know and I will review it.

liach

Looks good.

eirbjo · 2024-08-28T15:12:13Z

Thanks @LanceAndersen and @liach for your patient reviews.

I have updated the Specification section of the CSR and moved it to the Proposed state for an initial round of reviews:

https://bugs.openjdk.org/browse/JDK-8338663

bridgekeeper · 2024-09-25T15:39:54Z

@eirbjo This pull request has been inactive for more than 4 weeks and will be automatically closed if another 4 weeks passes without any activity. To avoid this, simply add a new comment to the pull request. Feel free to ask for assistance if you need help with progressing this pull request towards integration!

liach · 2024-09-25T15:41:42Z

@eirbjo Can you answer CSR review question about whether to just deprecate the constructor of ZipError for removal at the CSR?

eirbjo · 2024-10-01T06:58:33Z

@eirbjo Can you answer CSR review question about whether to just deprecate the constructor of ZipError for removal at the CSR?

Thanks for prodding! I have provided an answer to this question in the CSR: https://bugs.openjdk.org/browse/JDK-8338663#comment-14709333

ExE-Boss · 2024-10-01T15:50:59Z

The issue with removing ZipError (or any other exception class, such as SecurityException), is that existing classes which reference it in the exception_table of a Code attribute will fail verification.

This is different from referencing removed classes in method or field descriptors, those get treated as if they had the following declaration for the purposes of verification when determining whether checkcasts are necessary:

public abstract final class <name> extends Object {
}

src/java.base/share/classes/java/util/zip/ZipError.java

liach · 2024-10-01T16:20:55Z

@ExE-Boss Indeed, the removal is intended to be binary incompatible. Just like the example in [the fucntional removal of SecurityManager(https://openjdk.org/jeps/486), users can instrument or statically preprocess their java/util/zip/ZipError class entries to java/lang/InternalError similar to the preprocessing of System::exit calls.

eirbjo · 2024-10-04T15:35:35Z

The CSR has been approved to mark java.util.zip.ZipError deprecated, for removal with spec changes identical to the current state of this PR.

I plan to integrate this early next week. Just in case there are still any lingering concerns.

Thanks for all the patient reviews and comments, here and in the CSR!

eirbjo · 2024-10-07T18:11:02Z

As promised in the CSR, this change will be accompanied by a release note. A release note was drafted a while back, but I don't think anyone provided feedback.

Before integrating this change, I would love if someone could take a look at the release note and provide feedback. This way all aspects of this task can be completed:

https://bugs.openjdk.org/browse/JDK-8336843

eirbjo · 2024-10-08T10:07:26Z

/integrate

openjdk · 2024-10-08T10:08:32Z

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

f62dba3: 8341597: ZipFileInflaterInputStream input buffer size uses uncompressed size
59ac703: 8339220: [s390x] TestIfMinMax.java failure
ffb60e5: 8341594: Use Unsafe to coalesce reads in java.util.zip.ZipUtils
57c859e: 8339836: Open source several AWT Mouse tests - Batch 1
b6a4047: 8339982: Open source several AWT Mouse tests - Batch 2
45a6359: 8341668: Shenandoah: assert(tail_bits < (idx_t)BitsPerWord) failed: precondition
d996ca8: 8341581: Optimize BytecodeHelpers validate slot
4d50cbb: 8341278: Open source few TrayIcon tests - Set7
23f3ca2: 8330206: Bump minimum boot jdk to JDK 23
fc7244d: 8340713: Open source DnD tests - Set5
... and 570 more: https://git.openjdk.org/jdk/compare/5671f836039ef1683e3e9ce5b7cf0fa2f1860e2d...master

Your commit was automatically rebased without conflicts.

openjdk · 2024-10-08T10:08:37Z

@eirbjo Pushed as commit 7a1e832.

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

eirbjo added 2 commits August 20, 2024 12:18

Deprecate java.util.zip.ZipError for removal

5e63180

Update copyright year

ad40d13

openjdk bot added the csr Pull request needs approved CSR before integration label Aug 20, 2024

openjdk bot added the core-libs [email protected] label Aug 20, 2024

eirbjo marked this pull request as ready for review August 23, 2024 15:02

openjdk bot added the rfr Pull request is ready for review label Aug 23, 2024

liach reviewed Aug 24, 2024

View reviewed changes

Extend the deprecation note to mention that the error became obsolete…

eba159f

… in JDK 9 and to mention that code may be updated to catch the super class InternalError

liach approved these changes Aug 27, 2024

View reviewed changes

LanceAndersen reviewed Aug 27, 2024

View reviewed changes

src/java.base/share/classes/java/util/zip/ZipError.java Show resolved Hide resolved

eirbjo added 2 commits August 28, 2024 10:02

Simplify the deprecation note by focusing on behavior in the current …

dcffafe

…release

Merge branch 'master' into ziperror-deprecation

44a3284

LanceAndersen approved these changes Aug 28, 2024

View reviewed changes

liach approved these changes Aug 28, 2024

View reviewed changes

AlanBateman reviewed Oct 1, 2024

View reviewed changes

src/java.base/share/classes/java/util/zip/ZipError.java Show resolved Hide resolved

openjdk bot added ready Pull request is ready to be integrated and removed csr Pull request needs approved CSR before integration labels Oct 3, 2024

openjdk bot added the integrated Pull request has been integrated label Oct 8, 2024

openjdk bot closed this Oct 8, 2024

openjdk bot removed ready Pull request is ready to be integrated rfr Pull request is ready for review labels Oct 8, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

8336843: Deprecate java.util.zip.ZipError for removal #20642

8336843: Deprecate java.util.zip.ZipError for removal #20642

eirbjo commented Aug 20, 2024 •

edited by openjdk bot

Loading

eirbjo commented Aug 20, 2024

bridgekeeper bot commented Aug 20, 2024

openjdk bot commented Aug 20, 2024 •

edited

Loading

openjdk bot commented Aug 20, 2024

openjdk bot commented Aug 20, 2024

mlbridge bot commented Aug 23, 2024 •

edited

Loading

LanceAndersen commented Aug 23, 2024

ExE-Boss commented Aug 23, 2024

LanceAndersen commented Aug 23, 2024

liach commented Aug 23, 2024

eirbjo commented Aug 24, 2024

liach Aug 24, 2024

liach commented Aug 24, 2024 •

edited

Loading

LanceAndersen commented Aug 24, 2024

liach commented Aug 24, 2024

LanceAndersen commented Aug 25, 2024

liach commented Aug 26, 2024

eirbjo commented Aug 27, 2024 •

edited

Loading

liach left a comment

openjdk bot commented Aug 27, 2024

LanceAndersen left a comment

liach left a comment

eirbjo commented Aug 28, 2024

bridgekeeper bot commented Sep 25, 2024

liach commented Sep 25, 2024

eirbjo commented Oct 1, 2024

ExE-Boss commented Oct 1, 2024

liach commented Oct 1, 2024

eirbjo commented Oct 4, 2024

eirbjo commented Oct 7, 2024

eirbjo commented Oct 8, 2024

openjdk bot commented Oct 8, 2024

openjdk bot commented Oct 8, 2024

8336843: Deprecate java.util.zip.ZipError for removal #20642

8336843: Deprecate java.util.zip.ZipError for removal #20642

Conversation

eirbjo commented Aug 20, 2024 • edited by openjdk bot Loading

Progress

Issues

Reviewers

Reviewing

Webrev

eirbjo commented Aug 20, 2024

bridgekeeper bot commented Aug 20, 2024

openjdk bot commented Aug 20, 2024 • edited Loading

openjdk bot commented Aug 20, 2024

openjdk bot commented Aug 20, 2024

mlbridge bot commented Aug 23, 2024 • edited Loading

Webrevs

LanceAndersen commented Aug 23, 2024

ExE-Boss commented Aug 23, 2024

LanceAndersen commented Aug 23, 2024

liach commented Aug 23, 2024

eirbjo commented Aug 24, 2024

liach Aug 24, 2024

Choose a reason for hiding this comment

liach commented Aug 24, 2024 • edited Loading

LanceAndersen commented Aug 24, 2024

liach commented Aug 24, 2024

LanceAndersen commented Aug 25, 2024

liach commented Aug 26, 2024

eirbjo commented Aug 27, 2024 • edited Loading

liach left a comment

Choose a reason for hiding this comment

openjdk bot commented Aug 27, 2024

LanceAndersen left a comment

Choose a reason for hiding this comment

liach left a comment

Choose a reason for hiding this comment

eirbjo commented Aug 28, 2024

bridgekeeper bot commented Sep 25, 2024

liach commented Sep 25, 2024

eirbjo commented Oct 1, 2024

ExE-Boss commented Oct 1, 2024

liach commented Oct 1, 2024

eirbjo commented Oct 4, 2024

eirbjo commented Oct 7, 2024

eirbjo commented Oct 8, 2024

openjdk bot commented Oct 8, 2024

openjdk bot commented Oct 8, 2024

eirbjo commented Aug 20, 2024 •

edited by openjdk bot

Loading

openjdk bot commented Aug 20, 2024 •

edited

Loading

mlbridge bot commented Aug 23, 2024 •

edited

Loading

liach commented Aug 24, 2024 •

edited

Loading

eirbjo commented Aug 27, 2024 •

edited

Loading