Just a thought below to consider or ignore ;-)
> On Jul 2, 2019, at 11:27 PM, Stuart Marks <stuart.ma...@oracle.com> wrote:
>
> Hi Jaikiran,
>
> OK, good analysis. Rather a lot of issues for what seems like a simple patch,
> eh?
>
> - 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.
>
> - Subclasses
>
> I don't think there are any subclasses in the JDK, and I did some searching
> and I didn't find any subclasses "in the wild" either. I did find a few uses
> of these classes though. If these classes are rarely subclassed, we might be
> able to get away with changing the contract, though this does entail some
> risk.
we could consider leaving end() as is and just introduce close() which while
it duplicates what end() does, it reduces the chance of any potential
complaints.
We could then update the javadoc for end() to strongly suggest the use of
close() vs end()
>
> If we need to modify the subclasses used in tests, that's fair game though.
>
> - 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.
>
> 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.
>
> (This might be moot anyway. The condition where TWR closes a resource
> multiple times occurs in cases where closing a wrapper closes the wrapped
> resource, and both are TWR resource variables. But in my earlier example
>
> try (var inf = new Inflater();
> var iis = new InflaterInputStream(inputStream, inf)) {
> ...
> }
>
> and in fact all of {Inflater,Deflater}{Input,Output}Stream don't close the
> passed-in Deflater/Inflater, so multiple close() calls won't occur.)
>
> - Deprecation of end()
I don’t think we need to deprecate, just add a note to its javadoc encouraging
the use of close() going forward...
>
> 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).
>
> - 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>."
>
> **
>
> If you think the issues are settled enough, maybe it's time for you to take a
> stab at a patch. Up to you how to proceed with the "undefined" issue. If it's
> simple, great, I'd like to see it happen. But if it leads you off into the
> weeds, then feel free to drop it.
>
> 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
<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>
