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

[Peter Levart]

Hi Sherman,

On 09/30/17 02:02, Xueming Shen wrote:
> On 9/29/17, 1:18 PM, Peter Levart wrote:
>> 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?
>>
> Peter,
>
> I read those old emails back to 2011 on why we put the "Inflater" into 
> the streams as
> the value of the map entry. It appears the main (only) purpose is to 
> keep the finalization
> of <stream, inflater> in order. As I explained in previous email, 
> stream and its inflater
> can be phantom reachable at same round, if inflater gets 
> finalized/ended first, then it
> can no longer be reused/cached and has to be thrown away, which means 
> the caching
> mechanism is actually broken/not functioning. We do have use scenario 
> depends on it
> to avoid too many "new Inflater()' that might pressure the memory 
> usage. Putting the
> pair in a weakhashmap appears to solve this problem back then (the 
> ended() checks are
> still there just in case there is rare case, the inflater still gets 
> cleaned up first).
>
> The situation might be changed now in Cleaner case, as the cleaner 
> object itself now
> holds a strong reference, which in theory will/should prevent the 
> inflater from being
> phantom reachable before stream is cleaned, so we might no longer need 
> this
> <stream, inflater> pair in a map to have a "ordered" cleanup. Just 
> wanted to make sure
> my assumption is correct here.
>
> sherman
>

Right, the Inflater is captured by the cleaning function for the 
ZipFileInflaterInputStream and is kept strongly reachable until the 
function fires, which may happen automatically or manually. At that 
time, it is returned to the inflaters cache, which is rooted at the 
ZipFile instance and also captured by the ZipFile cleaning function - 
the Releaser, which is strongly reachable until fired. So I claim that 
there is no possibility for Inflaters used in ZipFile to ever get 
cleaned-up (ended) automatically by their cleaner functions as a result 
of getting phantom reachable. The situation has changed. It could even 
be beneficial to create a package-private Inflater constructor for this 
case, which creates Inflaters without cleaner registration.

Regards, Peter