Inputs on patch for JDK-8225763? Inflater and Deflater should implement AutoCloseable

[Stuart Marks]

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