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

[Xueming Shen]

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