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
