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

[Jaikiran Pai]

Hello Lance,

On 12/07/19 5:53 AM, Lance Andersen wrote:
> Hi Jaikiran,
>> I have now updated the javadoc of end() to be closer to the javadoc
>> of close().
>>
>>
>
> Its better but end() should include the wording 
>
> ————
> This method should be called when the compressor is no longer needed.
>
> ——————
>
> It could read
>
> This method or close() should be called when the compressor ……
>
>
> Also, thinking about the following in end():
>
> ———
> * Use of {@link #close()} is encouraged instead of this method.
> ———
>
> This might be better served as an @apiNote so it stands out

You are right, the wording you use is better. I have now updated the
javadoc of end() to use this wording. Furthermore, I have moved that
above part to the @apiNote.


>>>
>>>> - 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
>>
>> I haven't updated this in the latest webrev version and will wait for
>> us to come to a decision on how we word it for end().
>>
>
> OK

Given that we haven't yet come to a decision on how to word this, I went
ahead and attempted to improve this wording a bit. I have now moved it
into the @apiNote of both end() and close() javadoc and it now reads:

"Once the {@link #end()} or {@link #close()} have been called on a
compressor, the compressor may not be used for further operations. Doing
so may cause an exception to be thrown."

Of course, we can still change this to something else if this isn't yet
accurate enough.


>>>
>>> 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
>>>
>> I did not understand this. Did you mean I should update the @summary
>> part of these tests or was it the javadoc on these test methods?
>>
>>
> Sorry if my comment was confusing.  For example:
>
> ————
> public void testCloseThenEnd() throws Exception {
>  119         final Deflater simpleDeflater = new Deflater();
>  120         closeThenEnd(simpleDeflater);
>  121 
>  122         final OverrideClose overridenClose = new OverrideClose();
>  123         closeThenEnd(overridenClose);
>  124         // make sure close was called once
>  125         assertEquals(overridenClose.numTimesCloseCalled, 1, "Unexpected number of calls to close()");
>
> ——————
>
> As there is not an assert() for simpleDeflater, which is
> understandable,  the comment describing the test is not quite accurate
> should be slightly updated

OK, I get it now, thank you. Indeed what you note makes sense. I have
now updated both these new tests to move out the
"simpleDeflater/Inflater" testing outside into a new test method of its
own (along the lines to what you suggested in the previous reply) and
added comment to that test method to explain what it does. Let me know
if this looks better.

The new updated webrev is at
http://cr.openjdk.java.net/~jpai/webrev/8225763/4/webrev/

Thank you for your inputs so far.

-Jaikiran