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
