Hello Stuart,
On 03/07/19 8:57 AM, Stuart Marks wrote:
> Hi Jaikiran,
>
> OK, good analysis. Rather a lot of issues for what seems like a simple
> patch, eh?
Indeed, but I kind of expected that :)
>
> - Idempotency
>
> Yes, it looks to me like Inflater.end() and Deflater.end()
> implementations are idempotent. As you point out, overrides by
> subclasses might not be. We should be clear when we're talking about
> the specific implementatations of the end() methods in the Deflater
> and Inflater classes, or whether we're talking about the contracts
> defined by these method specifications on these classes and subtypes.
>
> The behavior of an implementation in a class can be specified with
> @implSpec without imposing this on the contract of subclasses. This is
> useful for subclasses that need to know the exact behavior of calling
> super.end(), and also for callers who know they have an instance of a
> particular class and not a subclass.
>
> The upshot is that while we might not have the end() method's contract
> specify idempotency, we can certainly say so in an @implSpec, if doing
> this will help. I'm not sure we'll actually need it in this case, but
> let's keep it in mind.
Thank you. Although not for end(), I have now used this @implSpec in the
first version of my patch[1].
>
> - Relationship between end() and close()
>
> I think close() should have more-or-less the same semantics as end(),
> namely, pretty much the way end() is specified now regarding releasing
> resources. But it should be allowable to call both in either order.
>
> try (var inf = new Inflater()) {
> ...
> # explicitly call end()
> inf.end();
> }
>
> I think this should allowed, but it shouldn't be required. The
> application can call either close() or end() and this will have the
> effect of releasing the native resources.
>
Agreed.
> A key question is whether close() should be specified to call end() --
> which is subject to being overridden by suclasses -- or whether
> close() is defined to do the same thing as the end() implementation in
> this class does. This can be implemented by taking the body of the
> current end() method and refactoring it into an internal method and
> then having both close() and end() call that internal method.
>
> If close() calls end() then AC+TWR might call close() twice, and
> therefore call end() twice, which might be a problem for subclasses.
> So to be really conservative we might want to do the internal-method
> refactoring to avoid this problem.
>
> On the other hand, suppose a subclass has extra resources it frees in
> its end() method, which then calls super.end(). Even though this class
> would inherit AC, using it in TWR would be a *bug* because close()
> would call the internal method to free the superclass resources, but
> this would leak the subclass resources. That sounds like a mistake to
> perpetuate in the API.
>
> It's possible for a subclass to override end() in such a way that's
> not idempotent, but I think this is unlikely. I'm leaning toward
> risking the small possibility of incompatibility in declaring end()
> idempotent, allowing close() simply to call end(). This makes TWR more
> useful in the long run, which seems like a better position to be in.
> Of course, if somebody turns up evidence that this would break a bunch
> of stuff, we should reconsider.
>
I thought about this a bit more and I think we should call end() from
close() instead of neither doing whatever end() is doing nor extracting
out the logic into a private method. The advantage of close() calling
end() is, (as you state), it allows the subclasses to correctly cleanup
any resources those subclasses are holding on to. Furthermore, I think
we can still make it such that the (potentially overridden) end()
doesn't get called more than once even if close() gets called multiple
times.
In the patch[1], I've implemented and documented close() such that it
calls end() atmost once.
>
> - Deprecation of end()
>
> I don't think deprecation of end() is useful. It'll just cause noise
> for people. Uses such as
>
> var deflater = new Deflater();
> try {
> ...
> } finally {
> deflater.end();
> }
>
> are valid and correct and needn't be changed (though using TWR is
> preferable, this is more of a style issue).
OK and I do agree about the noise the deprecation can cause in this case.
>
> - Undefined behavior after close()/end()
>
> OK, I'll admit this is possibly out of scope, but the line in the
> specs about "once end() is called, the behavior is undefined" rubs me
> the wrong way. Right now, most operations seem to call ensureOpen(),
> which throws NPE if the object has been closed. But "undefined" allows
> anything to happen, including filling the output buffer with garbage.
> That seems wrong. We might not want to specify what exception is
> thrown, but we might want to specify that *AN* exception is thrown --
> which must be a RuntimeException, since most methods don't declare any
> checked exceptions.
>
> In any case, the clause would have to be updated to say something like
> "Once this Deflater (Inflater) is closed, any subsequent operations
> will <whatever>."
In my patch I have attempted to clarify the end() method by taking your
input.
>
> **
>
> If you think the issues are settled enough, maybe it's time for you to
> take a stab at a patch.
The initial version of my patch is now ready at [1]. Here's a high level
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. 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.
- 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.
- A couple of new 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.
- I have run the tests under test/jdk/java/util/zip/ by running:
jtreg -jdk:build/macosx-x86_64-server-release/images/jdk
test/jdk/java/util/zip/
and they have come back passing (except for failures/errors in
java/util/zip/ZipFile/TestZipFile.java, java/util/zip/3GBZipFiles.sh and
java/util/zip/DataDescriptorSignatureMissing.java - those issues though
are unrelated to this change and expected to fail, based on what I see
in their test report logs)
- In addition, I have sneaked in an unrelated change in this patch, into
the Deflater.end() method:
public void end() {
synchronized (zsRef) {
+ // check if already closed
+ if (zsRef.address() == 0) {
+ return;
+ }
zsRef.clean();
input = ZipUtils.defaultBuf;
+ inputArray = null;
+ }
Unlike in the Inflater.end(), the Deflater.end() didn't previously have
the "inputArray = null". I have included it in this patch, since it
looks right to clean up that array too. If this isn't the right
time/patch to do it, please do let me know and I'll take that up separately.
Thank you very much for your inputs so far.
[1] http://cr.openjdk.java.net/~jpai/webrev/8225763/1/webrev/index.html
P.S: I can start a new official RFR thread in this mailing list, if
needed. Please do let me know.
-Jaikiran
>
> Note: US holiday weekend coming up; replies might take several days.
>
> s'marks
>
>
>
>
> On 6/29/19 4:16 AM, Jaikiran Pai wrote:
>>
>> On 29/06/19 4:31 PM, Jaikiran Pai wrote:
>>> 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.
>>
>> To be clear - I couldn't find any subclasses in the main source code of
>> JDK. There are a couple of subclasses in the tests
>> (ConstructInflaterOutput.java and ConstructDeflaterInput.java), but
>> those don't do much outside of the context of testing.
>>
>> -Jaikiran
>>
>>