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

[Jaikiran Pai]

Hello Stuart,

Thank you for the detailed response. Comments inline.

On 28/06/19 2:48 AM, Stuart Marks wrote:
> 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.
>
>
> 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. 

I did some checks in the current JDK code. From what I see, the Inflater
and Deflater do not have any subclasses within the JDK itself. So I
reviewed the end() method implementations of Inflater and Deflater.

The Inflater does the following:

public void end() {
        synchronized (zsRef) {
            zsRef.clean();
            input = ZipUtils.defaultBuf;
            inputArray = null;
        }
    }

The zfRef.clean() ends up calling the java.lang.ref.Cleanable#clean(),
which as per the javadoc says "Unregisters the cleanable and invokes the
cleaning action. The cleanable's cleaning action is invoked *at most
once* regardless of the number of calls to {@code clean}."

(emphasis added by me)

Based on this code in the Inflater#end() and the contract of
Cleanable#clean(), it appears that the Inflater#end() is effectively
idempotent.

Deflater#end() has similar code, except the inputArray = null. So that
too seems effectively idempotent. (side note - perhaps Deflater#end()
too should reset the inputArray to null?)

To be extra sure, I did a basic quick test by updating the TotalInOut
test to call the inflater/deflater end() methods more than once. No
issues with that change and test passed as expected.

Having said that, this only takes into account the JDK's implementation
of these methods and not of any sub-classes. Let's take a look at the
class level javadoc of Deflater as an example. It says:

* @apiNote
 * To release resources used by this {@code Deflater}, the {@link
#end()} method
 * should be called explicitly. Subclasses are responsible for the
cleanup of resources
 * acquired by the subclass. Subclasses that override {@link
#finalize()} in order
 * to perform cleanup should be modified to use alternative cleanup
mechanisms such
 * as {@link java.lang.ref.Cleaner} and remove the overriding {@code
finalize} method.

and now lets take the javadoc of its end method:

/**
     * Closes the compressor and discards any unprocessed input.
     *
     * This method should be called when the compressor is no longer
     * being used. Once this method is called, the behavior of the
     * Deflater object is undefined.
     */

Although there's an expectation of cleaning up resources in the end()
method by the sub-classes, there's no clear expectation on what's the
expected behaviour if end() is called more than once. In fact, the
javadoc of end() which states "undefined behaviour once end is called"
makes it such that, currently, the sub-classes could potentially have
issues if end() gets called more than once. So IMO, I don't think end()
can be expected to be idempotent. Of course, this is based on my limited
knowledge of this API (and JDK codebase in general) so it's possible I
may have missed something.


> 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().

For reasons I stated above, I don't think we can consider end() to be
idempotent. Having said that, we can probably still make/should make
close() idempotent.

In context of this, I think we should also consider this following case
which is a slight update to the example you previously posted:

try ( inf = new Inflater();) {

      # explicitly call end()
      inf.end();
}

Here, I explicitly end() the Inflater once and then let the
try-with-resources close() the Inflater. I think this _combination_ of
end() and close() should be idempotent. Perhaps, even this should be:

Inflater inf = new Inflater();

...

inf.close(); // close the inflater first

inf.end(); // and then end the inflater.

Of course, usage like this doesn't make sense, but it isn't prohibited
either.

On a slightly related note, should end() be deprecated in favour of
close() when we introduce this change? The reason I ask that is to see
if we want to move users away from the constructs like my above examples.


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

I just had a bit more detailed look at the javadoc and it looks like we
will have to update the class level javadoc of these classes which
currently have an @apinote which state:

"To release resources used by this {@code Deflater}, the {@link #end()}
method should be called explicitly......"

Once we introduce the close() method, this expectation of calling end()
explicitly won't be valid anymore.

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

Will do.


>
> 5) Might be a good idea to update the example code in the class doc to
> use try-with-resources.
>
Will do.

While we are at this, the javadoc of these classes don't say anything
about thread-safety of these classes. But I do see that the operations
in these classes use synchronized blocks, so I'm guessing there's an
expectation of these classes being thread safe? The reason I ask is so
that I can take this into account while coming up with the patch, if it
introduces any new members in these classes.

-Jaikiran