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

[Xueming Shen]

Hi Peter,

I like the idea of moving get/releaseInflter() into CleanableResource, 
though I doubt
how much it can really help the GC it should be a good thing to do to 
remove the strong
reference of ZipFile from stream's cleaner, and the code appears a 
little cleaner as well.

I was debating with myself whether or not the ZipFile.close() should 
throw an
UncheckedIOException (like those java.nio.file.Files methods do). But 
you're right it's
not good to simply ignoring them silently. Now I catch/unwarp the potential
UncheckedIOException from res.clean() and re-throw the embedded ioe.

I need to dig a little to recall the real reason of zsrc==null check in 
ensureOpen()
the comment does not sound updated. res.zsrc is not final and it is set 
to null
when clean up/close. So I keep it for now.

Most (if not all?) "minor nit" has been updated accordingly.

It seems like we might have have put the "finalize()" method back as an 
empty-body
method for compatibility reason. So not done yet :-)

http://cr.openjdk.java.net/~sherman/8185582/webrev/

thanks,
sherman

On 11/1/17, 5:04 AM, Peter Levart wrote:
> Hi Sherman,
>
> On 11/01/17 00:25, Xueming Shen wrote:
>> Hi Peter,
>>
>> After tried couple implementations it seems the most reasonable 
>> approach is to
>> use the coding pattern you suggested to move all pieces into ZSStream 
>> Ref. Given
>> we are already using the internal API CleanerFactory it is attractive 
>> to go a little
>> further to subclass PhantomCleanable directly (in which I would 
>> assume we can
>> save one extra object), but I think I'd better to follow the 
>> "suggested" clean usage
>> (to register a runnable into the cleaner) for now.
>>
>>   39 class ZStreamRef implements Runnable {
>>   40
>>   41     private LongConsumer end;
>>   42     private volatile long address;
>>   43     private final Cleanable cleanable;
>>   44
>>   45     ZStreamRef (Object owner, LongSupplier addr, LongConsumer 
>> end) {
>>   46         this.cleanable = 
>> CleanerFactory.cleaner().register(owner, this);
>>   47         this.end = end;
>>   48         this.address = addr.getAsLong();
>>   49     }
>>   50
>>
>> Similar change has been made for the ZipFile cleaner to follow the 
>> same coding
>> pattern. The "cleaner" is now renamed from Releaser to 
>> CleanableResource.
>>
>> http://cr.openjdk.java.net/~sherman/8185582/webrev/
>>
>> Thanks,
>> Sherman
>
> This looks mostly fine. One minor nit is that ZStreamRef.address field 
> does not have to be volatile. I checked all usages of 
> ZStreamRef.address() and all of them invoke it while holding a lock on 
> the ZStreamRef instance. The ZStreamRef.run() which modifies the 
> address is also synchronized. The other minor nit is that the 
> following ZipFile imports are not needed:
>
> import java.nio.file.Path;
> import java.util.Map;
> import jdk.internal.misc.JavaIORandomAccessFileAccess;
> import static java.util.zip.ZipConstants.*;
>
> (at least my IDEA colors them unused)
>
> Cleaner is modified in this webrev (just one empty line deleted) - 
> better not touch this file in this changeset
>
> Additional nits in ZipFile:
>
> - in lines 355, 356 you pull out two res fields into locals, but then 
> not use them consistently (lines 372, 389)
> - line 403 has a TAB character (is this OK?) and shows incorrectly 
> indented in my editor (should I set tab stops differently?)
> - line 457 - while should actually be if ?
> - in ensureOpen() the check for res.zsrc == null can never succeed 
> (CleanableResource.zsrc is a final field. If CleanableResource 
> constructor fails, there's no res object and there's no ZipFile object 
> either as ZipFile constructor does not do anything for this to escape 
> prematurely)
> - why don't you let IOExceptions thrown from CleanableResource.run() 
> propagate out of ZipFile.close() ?
>
> I would also rename static method Source.close(Source) to 
> Source.release(Source) so it would not clash with instance method 
> Source.close() which makes it ambiguous when one wants to use 
> Source::close method reference (both methods apply). I would also make 
> static methods Source.get() and Source.release(Source) package-private 
> (currently one is public and the other is private, which needs 
> compiler bridges to be invoked) and both are in a private nested class.
>
> Inflater/Deflater/ZipFile now follow the coding pattern as suggested. 
> But ZipFileInflaterInputStream still does not. It's not critical since 
> failing to register cleanup which releases the inflater back into 
> cache would simply mean that Inflater employs its own cleanup and ends 
> itself.
>
> And now another thing I would like to discuss. Why an initiative for 
> using Cleaner instead of finalization()? Among drawbacks finalization 
> has one of the more troubling is that the tracked referent survives 
> the GC cycle that initiates its finalization reference processing 
> pipeline, so the GC may reclaim the object (and referenced objects) 
> only after the finalize() has finished in yet another GC round. 
> Cleaner API separates the tracked object from the cleanup function and 
> state needed to perform it into distinct instances. The tracked object 
> can be reclaimed and the cleanup reference processing pipeline 
> initiated in the same GC cycle. More heap may be reclaimed earlier.
>
> Unless we are careful and create a cleaning function for one tracked 
> object which captures (directly or indirectly) another object which 
> registers its own cleaning function but we don't deal with explicit 
> cleaning of the 2nd object in the 1st cleaning function.
>
> Take for example the ZipFileInflaterInputStream's cleaning function. 
> It captures the ZipFile in order to invoke ZipFile.releaseInflater 
> instance method. What this means is that ZipFile will be kept 
> reachable until all ZipFileInflaterInputStream's cleaning functions 
> have fired. So we are back to the finalization drawback which needs at 
> least 2 GC cycles to collect and clean-up what might be done in one go.
>
> I suggest moving the getInflater() and releaseInflater() from ZipFile 
> into the CleanableResource so that ZipFileInflaterInputStream's 
> cleaning function captures just the CleanableResource and not the 
> ZipFile. ZipFile therefore may become eligible for cleanup as soon as 
> all opened input streams become eligible (but their cleaning functions 
> need not have fired yet). CleanableResource.run() (the ZipFile 
> cleaning function) and CleanableResource.releaseInflater() (the 
> ZipFileInflaterInputStream's cleaning function) may therefore be 
> invoked in arbitrary order (maybe even concurrently if one of them is 
> explicit cleanup and the other is automatic), so code must be prepared 
> for that.
>
> I have tried to capture all above in a modified webrev (taking your 
> last webrev as a base):
>
> http://cr.openjdk.java.net/~plevart/jdk10-dev/8185582_ZIP.cleaner/webrev.03/
>
>
> Regards, Peter
>