On 6/26/19 9:28 PM, Jaikiran Pai wrote:
I am looking to contribute a patch for the enhancement noted in
https://bugs.openjdk.java.net/browse/JDK-8225763. The change itself
looks relatively straightforward to me and here's what I plan to do:
1. Have both java.util.zip.Inflater and java.util.zip.Deflater start
implementing the AutoCloseable interface
2. Have the close() method call the end() method
3. Add javadoc to the close() implementation to mention that the end()
method gets called
4. Add test(s) to verify that the close indeed actually "ends" the
inflater/deflater
5. This is more of a question - do we have to update the class level
javadoc of both these classes to make a mention that these classes have
started implementing the AutoCloseable starting version 14 (or whichever
version this change makes into) of Java?
Any other inputs/suggestions on what else might be needed in this change?
Hi Jaikiran,
Thanks for picking this up. There are some small wrinkles with this, but I hope
nothing that will prevent it from happening.
1) It's odd that Deflater/Inflater weren't retrofitted to AutoCloseable back in
Java 7 when AC and try-with-resources were introduced. It might have merely been
an oversight, as the method is named "end" instead of "close". Joe Darcy, who
drove much of the retrofitting work, said that he searched for classes that had
a "close" method to generate the list of retrofit candidates. If so, that would
have missed the "end" method.
On other hand, there might be some other reason that's not obvious that makes
retrofitting to AC impractical or incorrect or something. I've asked around
internally and nobody can think of any fundamental issues, though. Perhaps
somebody else on the mailing list might know of something?
2) Alan already mentioned this, but we need to consider the issue of idempotent
close() or end() carefully. It's not strictly required, but it would be nice. I
had taken a quick look at end() and I *thought* it was idempotent already, but a
more careful analysis needs to be done. Idempotent close is an issue when using
multiple, wrapped resources, and where an outer close() closes the inner one as
well.
If Inflater were retrofitted to implement AC, oddly, the following would not
require idempotent close:
try (var inf = new Inflater();
var iis = new InflaterInputStream(inputStream, inf)) {
...
}
The reason is that InflaterInputStream does NOT close the Inflater if it's
passed one explicitly, but it DOES close it if it created the Inflater itself.
This makes sense, but unfortunately it doesn't appear to be documented well.
It's probably a potential source of errors in that the doc says "releases any
system resources associated with the stream" but it doesn't close the Inflater
if one was passed in explicitly. Oh well, probably too late to change this now.
If we were to make something idempotent, it's not clear whether idempotency
should apply to just close() or both to end() and close().
3) Good, you're thinking about spec updates. Clearly this will need to reflect
whatever is decided about idempotency.
Regarding a note in the specification about retrofitting AC, I don't necessarily
think anything is necessary in the spec itself other than "@since 14" (or
whatever) on the close() method. There's no @since on an "implements" clause.
However, a release note would be appropriate for this.
Also, a CSR request should be filed for the spec change and it needs to be a
approved before the change goes in. This isn't a huge deal but it's required for
spec changes, it involves some writing, and a few additional days for
review/approval.
We can help you with release notes and the CSR process when the time comes.
4) Good, you're also thinking about testing. One test you might consider
updating is test/jdk/java/util/zip/TotalInOut.java.
5) Might be a good idea to update the example code in the class doc to use
try-with-resources.
Again, thanks for picking this up.
s'marks
