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

[Xueming Shen]

On 10/28/17, 7:16 AM, Florian Weimer wrote:
> * Xueming Shen:
>
>> 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
> In ZipFile:
>
> 387                 Inflater inf = getInflater();
> 388                 InputStream is = new ZipFileInflaterInputStream(in, inf, (int)size,
> 389                                          () ->  releaseInflater(inf));
> 390                 synchronized (streams) {
> 391                     streams.add(is);
> 392                 }
>
> Doesn't this leak the inflater if Cleaner.register() and thus the
> ZipFileInflaterInputStream constructor throw?

Hi Florian, thanks for reviewing.

The answer is "yes", if Cleaner.register() fails and throws a runtime 
exception ( internal
error probably?)  somewhere inside its implementation. I'm now an expert 
on Cleaner's
implementation, but the "register" operation looks straightforward, 
creating the phantom
ref object and "inserting" the newly created phantom ref into the queue, 
which appears
unlikely to fail easily. The "assumption" of taking the approach to move 
away from finalize()
to cleaner is that "Cleaner.register()" should work as reliable as 
finalizer. But in situation it
really fails, I would assume there is really nothing can be done to save 
the world here. Similar
to the situation like "free()" fails (for the "calloc()/free()" pair) 
and the memory to be
deallocated might be left "un-freed"?

In the latest version (this webrev) the inflater's default "cleaner" is 
turned off via a package
private constructor, with the assumption that it is guaranteed that the 
inflater will always be
putback into the "inflaterCache" by the corresponding ZFIIS cleaner and 
all inflaters inside
"inflaterCache" is guaranteed to be cleaned up via the ZipFile cleaner. 
So yes arguably we are
less safe by removing that (if this is the concern) But if 
Cleaner.register() here can fail, there
is no guarantee inflater's default Cleaner.register() will not fail.

It might be possible to try-catch to expect Cleaner.register might fail, 
but I doubt it's really
necessary here and it is the recommended usage of Cleaner?

Thanks!
-Sherman