Inputs on patch for JDK-8225763? Inflater and Deflater should implement AutoCloseable

[Stuart Marks]

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.

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 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
> 
>