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.forums2...@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.ander...@oracle.com <mailto:lance.ander...@oracle.com>
