-
Notifications
You must be signed in to change notification settings - Fork 5.8k
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
8225763: Inflater and Deflater should implement AutoCloseable #19675
Conversation
/csr needed |
👋 Welcome back jpai! A progress list of the required criteria for merging this PR into |
@jaikiran 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 18 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 |
@jaikiran has indicated that a compatibility and specification (CSR) request is needed for this pull request. @jaikiran please create a CSR request for issue JDK-8225763 with the correct fix version. This pull request cannot be integrated until the CSR request is approved. |
Webrevs
|
@Override | ||
public void close() { | ||
synchronized (zsRef) { | ||
// check if already closed |
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.
Should we comment // in case subclasses override the closed check in end()
instead? Otherwise the duplicate comments and checks are confusing.
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.
Hello Chen, we want the close() to be idempotent irrespective of whether or not end() is overridden. That check in close() and the code comment on that line is to indicate that. If the comment is adding to confusion, I can remove it - I don't think it's a must to have it to understand the check.
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 mean, we have updated the default implementation of end() to be idempotent. It now just appears confusing to readers that close() seems to perform redundant checks for idempotence until they realize subclasses can override end() to make it non-idempotent.
My suggestion is that our comment should say why we have this check here even though there's already an identical check in end().
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.
My suggestion is that our comment should say why we have this check here even though there's already an identical check in end().
I now see what you mean. I'll update this appropriately tomorrow.
@Override | ||
public void close() { | ||
synchronized (zsRef) { | ||
// check if already closed |
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.
Same remark, we should mention subclass override close risk
@@ -0,0 +1,286 @@ | |||
/* | |||
* Copyright (c) 2019, Oracle and/or its affiliates. All rights reserved. |
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.
* Copyright (c) 2019, Oracle and/or its affiliates. All rights reserved. | |
* Copyright (c) 2024, Oracle and/or its affiliates. All rights reserved. |
Same for InflaterClose.java
.
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.
Hello Chen, these tests were actually introduced in a RFR back in 2019, so the original year is correct. I've now converted these tests to junit and updated the PR and updated the copyright years as well.
Hi Jai, thanks for picking this up. Several points of discussion here. Nothing fatal, but as I said in 2019 :-) just a few wrinkles to iron out. I looked mainly at Deflater, but I think the issues also all apply to Inflater too.
I think the class specification needs to be clearer about the positioning of end() and close(). The end() method has done the real work of "closing" since the classes were introduced. We're retrofitting them to to have a close() method that's essentially just a synonym for end(), and it's still perfectly legal for clients and subclasses to call end() instead of close(). I think we need to say that close() is present mainly to support AutoCloseable and try-with-resources.
I don't think that "may throw an exception" is strong enough. Also, on these classes (not subclasses) end() is idemopotent, isn't it? If so, this should be specified. The end() specs need to say something like, closes and releases resources, etc., henceforth operations will throw an exception (which? -- see below) but that subsequent calls to end() do nothing.
Oddly, it doesn't seem to be specified anywhere what exceptions are thrown if operations are attempted on a closed object. And the implementation actually throws NPE??!? Ideally we should specify what exception is thrown in this circumstance, but NPE seems wrong. Maybe we want to change the behavior to throw a different exception type. IllegalStateException seems like a reasonable candidate.
The 2019 discussion kind of assumed that we want close() to be idempotent. I'm not sure this is necessary. Alan said the implementation might need to be "heroic" but Peter Levart's arrangement (carried through here, apparently) isn't terribly bad. And @liach suggests more comments on this code, quite reasonably... but the comment begs the question, why do we need to avoid calling end() more than once? I'm rethinking the need for close() to be idempotent. Maybe close() should simply call end(). It would simplify the implementation and the specs -- the implSpec would simply say that it calls the end() method. This would only be a problem if a) some arrangement of TWR ends up calling close() twice, which apparently it doesn't; b) there is a subclass; and c) the subclass's end() method is not idempotent. As far as I can tell we haven't found any example of any of these. It thus seems to me that having extra logic in close() to avoid calling end() more than once is solving a non-problem. It also fits more closely to the model of "close() is simply a synonym for end()". |
@jaikiran 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! |
Please keep open, I'm running some experiments for the proposed alternative and I'll update this PR shortly. |
@jaikiran 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! |
@jaikiran This pull request has been inactive for more than 8 weeks and will now be automatically closed. If you would like to continue working on this pull request in the future, feel free to reopen it! This can be done using the |
/open |
@jaikiran This pull request is now open |
Thank you Stuart for your inputs.
I've now updated the class level javadoc of both Inflater and Deflater to be more explicit on the purpose of close() method and how it relates to the end() method.
I've updated both the end() and close() method javadocs. It now states that end() is idempotent and at the same time states that calling several other methods on a closed Inflater/Deflater will throw a specific exception (noted below). I copied over the sentence which states "Several other methods defined..." from an existing Java SE API.
Yes, it is indeed odd that it's throwing a
I ran various corpus experiments. Based on that what I have noticed is that, in that corpus, there are extremely few (less than 10) classes which override end() of either the Inflater or Deflater class. Furthermore, all these implementations have implemented their end() in a manner that calling it more than once will act as a no-op for subsequent invocations. So I think, like you note, we should just specify that close() calls end() and stop trying to prevent end() being called more than once. Given this, I've now updated the PR to do just that and also updated the new tests to match this specification updates. tier testing is currently in progress for this current state of the PR. If the current state of the PR looks reasonable, then I'll focus on the CSR text for these changes. |
* @since 24 | ||
*/ | ||
@Override | ||
public void close() { |
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.
Can/should this method be final? The real/original cleanup method is end
and if both close()
and end()
are overriddable, there may be some confusion about which to implement.
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.
Hello Roger, I think that's a good idea. At least until there is a real reason for close()
to be overridden by subclasses. I went back and checked the mailing list discussions (linked in this PR description) and I don't think marking close()
as final
has been suggested or rejected before. I will give others a chance to provide their inputs before I do this change.
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 agree with marking these close methods as final as long as the previous corpus search did not find any user-defined close method in subclasses.
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.
Hello Chen, corpus analysis shows that there are no subclasses of Inflater/Deflater which have a close()
method.
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.
Great news! Feel free to make their close final.
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 have updated the PR to follow Roger's suggestion of marking this new close()
method on Inflater
and Deflater
classes as final
.
* @since 24 | ||
*/ | ||
@Override | ||
public void close() { |
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.
Ditto, should this be final
to be clear about what should be overridden to perform cleanup.
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.
Done.
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.
Inflater/Deflater date from JDK 1.1 so I think there is some risk to adding a final no-arg close method. I understand that a corpus analysis has been done, and the intention of making it final is for subclasses to know which method to override, but it doesn't take from the possibility that it might break subclasses developed years ago. If it goes ahead as reviewed, then we'll to do some outreach and be prepared to back out this change in the event that breakage is reported before GA.
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.
Hello Alan,
If it goes ahead as reviewed, then we'll to do some outreach and be prepared to back out this change in the event that breakage is reported before GA.
I agree that if the final close()
causes issues, then backing this change out and reconsidering a non-final close()
would be the right thing to do. Once/if this gets integrated into 24, I will work with the outreach team to have this change noted in their EA mails.
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.
Adding a final close method is both source and binary incompatible so I don't think the CSR can be approved without more justification. So I think the issues of introducing a non-final close should be examined again.
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.
Hello Alan, I've now reverted the change which had marked this new close()
method on Inflater
and Deflater
as final
. With this update to this PR, this method is no longer final
. Given this change, one additional sentence has been added to the Inflater/Deflater class level javadoc, in @apiNote
section providing guidance to the subclasses on which method to override for cleaning up resources they hold:
Subclasses should override {@linkplain #end()} to clean up the resources acquired by the subclass.
No other changes have been done.
Requesting your and other's inputs if this updated change looks reasonable.
Please keep open |
@@ -799,6 +812,8 @@ public int getAdler() { | |||
* and therefore cannot return the correct value when it is greater | |||
* than {@link Integer#MAX_VALUE}. | |||
* | |||
* @throws IllegalStateException if the Deflater is closed |
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.
@throws should follow @return; (Style notes in https://www.oracle.com/technical-resources/articles/java/javadoc-tool.html).
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.
Done - I've updated the PR to fix this and the other places that were introduced in this PR to follow that style guide.
@@ -829,6 +845,8 @@ public long getBytesRead() { | |||
* and therefore cannot return the correct value when it is greater | |||
* than {@link Integer#MAX_VALUE}. | |||
* | |||
* @throws IllegalStateException if the Deflater is closed |
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.
Ditto @throws
throws should follow @return
@@ -646,6 +660,8 @@ public long getBytesRead() { | |||
* and therefore cannot return the correct value when it is greater | |||
* than {@link Integer#MAX_VALUE}. | |||
* | |||
* @throws IllegalStateException if the Inflater is closed |
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.
Ditto @throws
should follow @return
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.
Overall looks good Jai.
Roger is right on moving the @throws IAE further down
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.
Thanks for the updates.
Thank you all for the reviews and the inputs. The CSR has been approved. I'll go ahead and integrate this shortly. |
@@ -49,29 +49,33 @@ | |||
* thrown. | |||
* <p> | |||
* This class deflates sequences of bytes into ZLIB compressed data format. | |||
* The input byte sequence is provided in either byte array or byte buffer, | |||
* The input byte sequence is provided in either byte array or {@link ByteBuffer}, |
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.
We should probably fix this sentence to say "in either a byte array or ...".
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.
Fixed.
* To release resources used by the {@code Deflater}, applications must close the | ||
* {@code Deflater} by calling either the {@link #end()} or the {@link #close()} method. | ||
* After the {@code Deflater} has been closed, subsequent calls to several methods | ||
* of the {@code Deflater} will throw an {@link IllegalStateException}. |
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.
This paragraph uses the definite article but there isn't a specific Deflater to speak of, and it's not a singleton. The first sentence of this paragraph might be better if re-worded "To release the resources used a Deflater, an application must close it by invoking its end() or close() method".
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 have reworded the sentence to follow your input. Addtionally, I've removed the second sentence, since as you note, relevant methods on the Inflater/Deflater (as part of this PR) already have been updated to state that they throw an IllegalStateException
if the Inflater/Deflater is already closed.
* Closes and releases the resources held by this {@code Deflater} | ||
* and discards any unprocessed input. | ||
* <p> | ||
* If this method is invoked multiple times, the second and subsequent calls do nothing. |
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 this could be clearer if you replace this sentence with "If the Deflater is already closed then invoking this method has no effect."
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.
Done.
* This class implements {@link AutoCloseable} to facilitate its usage with | ||
* {@code try}-with-resources statement. The {@linkplain Deflater#close() close() method} simply | ||
* calls {@code end()}. Subclasses should override {@linkplain #end()} to clean up the | ||
* resources acquired by the subclass. |
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 wonder if the note for subclasses should move to the end method as implSpec, it looks out of place in the class API note.
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 believe an @implSpec
only restricts the implementation of this immediate method, such as those seen on default methods, and has no impact on the overrides. If we need to specify something for overrides to abide to, I believe they usually stay in the API specification itself.
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 wonder if the note for subclasses should move to the end method as implSpec, it looks out of place in the class API note.
Stuart, in his inputs here #19675 (comment) and here #19675 (comment) had noted that it would be useful to add this guidance to subclasses. But I see that he wasn't fully convinced whether it must be a apiNote or something else. Given what you note about moving this detail for subclasses to a @implSpec
on the end() method, I have now updated the PR accordingly.
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.
Hello Chen,
I believe an @implSpec only restricts the implementation of this immediate method, such as those seen on default methods, and has no impact on the overrides.
Based on the details about these tags here https://openjdk.org/jeps/8068562, I think the @implSpec
is the most relevant one for this text:
Interface implementors or class subclassers use the information here in order to decide whether it is sensible or necessary to override a particular method
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.
Thanks for the updates, looks good now.
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.
Latest Updates look good Jai
Thank you all for the reviews. tier1, tier2 and tier3 tests pass with this change. I'll now go ahead and integrate this. |
/integrate |
Going to push as commit 36b7abd.
Your commit was automatically rebased without conflicts. |
Can I please get a review of this enhancement which proposes to enhance
java.util.zip.Deflater/Inflater
classes to now implementAutoCloseable
?The actual work for this was done a few years back when we discussed the proposed approaches and then I raised a RFR. At that time I couldn't take this to completion. The current changes in this PR involve the implementation that was discussed at that time and also have implemented the review suggestions from that time. Here are those previous discussions and reviews:
https://mail.openjdk.org/pipermail/core-libs-dev/2019-June/061079.html
https://mail.openjdk.org/pipermail/core-libs-dev/2019-July/061177.html
https://mail.openjdk.org/pipermail/core-libs-dev/2019-July/061229.html
To summarize those discussions, we had concluded that:
Deflater
andInflater
will implement theAutoCloseable
interfaceclose()
implementation we will invoke theend()
method (end()
can be potentially overridden by subclasses).close()
will be specified and implemented to be idempotent. Callingclose()
a second time or more will be a no-op.end()
and thenclose()
, although uncommon, will also support idempotency and thatclose()
call will be a no-op.close()
and thenend()
will not guarantee idempotency and depending on the implementing subclass, theend()
may throw an exception.New tests have been included as part of these changes and they continue to pass along with existing tests in tier1, tier2 and tier3. When I had originally added these new tests, I hadn't used junit. I can convert them to junit if that's preferable.
I'll file a CSR shortly.
Progress
Issues
Reviewers
Reviewing
Using
git
Checkout this PR locally:
$ git fetch https://git.openjdk.org/jdk.git pull/19675/head:pull/19675
$ git checkout pull/19675
Update a local copy of the PR:
$ git checkout pull/19675
$ git pull https://git.openjdk.org/jdk.git pull/19675/head
Using Skara CLI tools
Checkout this PR locally:
$ git pr checkout 19675
View PR using the GUI difftool:
$ git pr show -t 19675
Using diff file
Download this PR as a diff file:
https://git.openjdk.org/jdk/pull/19675.diff
Using Webrev
Link to Webrev Comment