RFR JDK-8185582, Update Zip implementation to use Cleaner, not finalizers

[Peter Levart]

Hi Sherman,

I looked into ZipFile as promised.

One thing I noticed is that FinalizeZipFile.java test fails compilation:

test/jdk/java/util/zip/ZipFile/FinalizeZipFile.java:49: error: unreported exception Throwable; must be caught or declared to be thrown
             super.finalize();
                           ^

(the overridden finalize() in InstrumentedZipFile should now declare 
throws Throwable, since it overrides Object.finalize() and not 
ZipFile.finalize() which is gone).


The other thing I noticed is that Releaser 1st closes the streams (that 
are still reachable via streams WeakHashMap) and also ends the 
associated inflaters. But closing the stream will already release the 
inflater (in case it is a ZipFileInflaterInputStream) into the inflaters 
cache and the cache is scanned and inflaters ended later.

So we don't need a stream -> inflater association outside the stream in 
the form of WeekHashMap. But we still need to keep the set of input 
streams weakly reachable from ZipFile in case we want to close the 
ZipFile explicitly (and there is no harm although useless if this also 
happens as a result of automatic ZipFile cleaner processing).

This could be implemented in a form of:

final Set<InputStream> streams = Collections.newSetFromMap(new 
WeakHashMap<>());

I also noticed that it is useless to test whether the inflater is 
ended() when obtaining it from or releasing it into cache if the code 
keeps the invariant that it never ends inflaters while they are still in 
cache or associated with the open stream (the only place where inflaters 
should be ended explicitly is in the Releaser). To make this even more 
obvious, it might be good to move the obtaining/releasing logic directly 
into the ZipFileInflaterInputStream constructor which would be passed a 
reference to the inflatersCache instead of the Inflater instance.

Here's what I have in mind (I cahnged just the ZipFile and FinalizeZipFile):

http://cr.openjdk.java.net/~plevart/jdk10-dev/8185582_ZIP.cleaner/webrev.01/

What do you think?

Because Inflaters used in ZipFile will never be automatically ended by 
their own cleaners (they are kept strongly reachable in the cache of 
inflaters, which is strongly reachable from the registered ZipFile 
cleanup function), it might be useful to add a special package-private 
constructor to Inflater which would not register its own cleaner. But 
this could be left as an exercise for some later time.

Regards, Peter


On 09/28/17 01:41, Xueming Shen wrote:
> Thanks Mandy!
>
> I removed the ln#657-#663, and updated the @apiNote in deflter, inflater
> and zipfile accordingly, mainly combined your comment and the approach
> for the fis/fo. they are "simpler" and straightforward now, at least 
> for me.
>
> https://bugs.openjdk.java.net/browse/JDK-8187485
> http://cr.openjdk.java.net/~sherman/8185582/webrev
>
> -Sherman
>
> On 9/27/17, 3:08 PM, mandy chung wrote:
>>
>> Comment on the CSR:
>>
>> On 9/26/17 11:35 PM, Xueming Shen wrote:
>>>
>>> csr: https://bugs.openjdk.java.net/browse/JDK-8187485
>>>
>> I think the @apiNote can be simpler.
>>
>> Deflater (similar comment for Inflater)
>> |  * @apiNote
>>   * In earlier versions, the {@link Object#finalize} method was 
>> overridden
>>   * to call the {@link #end} method when a {@code Deflater} object
>>   * becomes unreachable.
>>   * The {@link #finalize} method is no longer defined.  If a subclass
>>   * overrides||the {@code end} method, the overridden {@code end} method
>>   * is no longer invoked.
>>   *<p>
>>   * It is strongly recommended to explicitly call {@code end} to
>> ||  * discard any unprocessed input promptly to free up resources
>> |  * when|||the compressor|is no longer in use.|
>>
>>
>> |ZipFile
>>     * @apiNote
>> |  * In earlier versions, the {@link Object#finalize} method was 
>> overridden
>>   * to call the {@link #close} method when a {@code ZipFile} object
>>   * becomes unreachable.|
>> |  * The {@link #finalize} method is no longer defined.  If a subclass
>>   * overrides||the {@code close} method, the overridden {@code close} 
>> method
>>   * is no longer invoked.|
>>   *<p>
>> |  * The recommended cleanup for|||{@code ZipFile}|  is to explicitly 
>> call {@code close}
>>   * or use try-with-resources.|
>>
>>   657      *<p>
>>   658      * Since the time when the resources held by this object 
>> will be released
>>   659      * is undetermined, if this method is not invoked 
>> explicitly, it is strongly
>>   660      * recommended that applications invoke this method as soon 
>> they have
>>   661      * finished accessing this {@code ZipFile}. This will 
>> prevent holding up
>>   662      * system resources for an undetermined length of time.
>>   663      *<p>
>>
>> I would suggest to drop this paragraph.  @apiNote and @implNote in
>> class spec cover that.
>>
>> Mandy
>> || 
>