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
||


Reply via email to