RFR [14] JDK-8225763 Inflater and Deflater should implement AutoCloseable

[Lance Andersen]

Hi Jaikiran,

Again, thank you for your efforts here.

Is there a CSR for this yet?  This will need to approved prior to moving forward with committing the feature.

I can sponsor once everything has been approved and finalized.




———
@implSpec This method is a no-op if this compressor has already
 886      * been previously closed,

————

Please remove “already” in both the close() and end() methods.  I believe the preference is the @implSpec and its relatives are on their own line as in https://openjdk.java.net/jeps/8068562 <https://openjdk.java.net/jeps/8068562> and was done for @apiNote earlier

 - Undefined behavior after close()/end()

I am not convinced the new wording is an improvement and I know Stuart was not thrilled with the existing wording.  However given the classes may be subclassed, I am not sure we  can truly specify the behavior which could be why the original authors used that wording.   Stuart thoughts?


Outside of the @ImplSpec, I am not sure the initial wording for end() and close() really need to differ:

end():

	Closes the decompressor and discards any unprocessed input.

close():

Releases resources held by this decompressor and discards any unprocessed input.This method should be called when the decompressor is no longer needed



> On Jul 9, 2019, at 9:18 AM, Jaikiran Pai <jai.forums2013 at gmail.com> wrote:

> 
> Can I please get a review and a sponsor for the patch that implements
> the enhancement noted in https://bugs.openjdk.java.net/browse/JDK-8225763 ?
> 
> There has been a recent discussion about this change in the
> core-libs-dev mailing list here[1]. The latest version of the patch for
> this change is now available at [2].
> 
> Here's a summary of what's contained in this patch:
> 
> - The Inflater/Deflater class now implements AutoCloseable
> 
> - The class level javadoc of both these classes has been to updated to
> use newer style of code, with try-with-resources, for the example
> contained in that javadoc. The @apiNote in these class level javadoc has
> also been updated to mention the use of close() method for releasing
> resources.
> 
> - The javadoc of end() method in both these classes has been updated to
> encourage the use of close() method instead of this one. It now also has
> a @implSpec which states that it's a no-op if called more than once.
> 
> In addition, this javadoc has also been updated to replace the
> "undefined behaviour" statement with a mention that the usage of the
> Inflater/Deflater instance after a call to end() may throw an exception
> in those subsequent usages. Please note that, there's no such explicit
> mention in the javadoc of the (newly added) close() method because IMO,
> it isn't needed for close() since I think it's kind of implied that a
> closed resource can no longer be used for further operations.

We need to be specific in close()  also for clarity
> 
> - The new close() method has been added along with javadoc which uses an
> @implSpec to state that it internally calls end()
> 
> - TotalInOut.java test has been updated to use the new
> try-with-resources construct for the inflater and deflater it uses.

Please update @biug to include the bug number
> 
> - A couple of new (testng) test classes have been added to test various
> scenarios where close() and end() method get called. These test mostly
> focus on ensuring that the close() and end(), either implicitly or
> explicitly, get called the right number of times on the subclasses of
> Inflater/Deflater.

Overall they look OK.  In your tests, you are testing the number of calls for the sub-classes not for Deflate/Inflate so I would either update your comments to clarify that or pull them into their own test methods


Again, thank you for your efforts here.

Best
Lance
 <http://oracle.com/us/design/oracle-email-sig-198324.gif>
 <http://oracle.com/us/design/oracle-email-sig-198324.gif> <http://oracle.com/us/design/oracle-email-sig-198324.gif>
 <http://oracle.com/us/design/oracle-email-sig-198324.gif>Lance Andersen| Principal Member of Technical Staff | +1.781.442.2037
Oracle Java Engineering 
1 Network Drive 
Burlington, MA 01803
Lance.Andersen at oracle.com <mailto:Lance.Andersen at oracle.com>