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

[Xueming Shen]

On 10/28/17, 10:47 AM, Peter Levart wrote:
> Hi Florian,
>
> On 10/28/17 16:16, Florian Weimer wrote:
>> * Xueming Shen:
>>
>>> 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?
>
> Thanks for being alert. I think that all that can be thrown between 
> getInfalter() call and streams.add() successfully returning is 
> OutOfMemoryError or StackOverflowError. OOME can be thrown anywhere, 
> not only as a consequence of Cleaner.register(). I see following 
> potential places where allocation may be taking place:
>
> - constructing the lambda instance (line 389)
> -  allocating ZipFileInflaterInputStream object
> - multiple places in ZipFileInflaterInputStream.<init> and 
> InflaterInputStream.<init> (including but not limited to 
> Cleaner.register())
> - streams.add() (line 391)
>
> Can we do anything about it? Let's see. For example:
>
> Inflater inf = getInflater();
> InputStream is;
>
> try {
>     is = new ZipFileInflaterInputStream(in, inf, (int)size, () -> 
> releaseInflater(inf));
> } catch (OutOfMemoryError e) {
>     releaseInflater(inf);
>     throw e;
> }
>
> try {
>     synchronized (streams) {
>         streams.add(is);
>     }
> } catch (OutOfMemoryError e) {
>     try { is.close(); } catch (IOException ignore) {}
> }
>
> ...but, even releaseInflater(inf) may throw OOME (in line 470):
>
>  467     private void releaseInflater(Inflater inf) {
>  468         inf.reset();
>  469         synchronized (inflaterCache) {
>  470             inflaterCache.add(inf);
>  471         }
>  472     }
>
> So we may change it to the following, which would make it more robust 
> for other uses too:
>
> private void releaseInflater(Inflater inf) {
>    inf.reset();
>    try {
>        synchronized (inflaterCache) {
>            inflaterCache.add(inf);
>        }
>    } catch (OutOfMemoryError e) {
>        inf.end();
>        throw e;
>    }
> }
>
> ...and that's it for OOME, hopefully.
>
> But what about StackOverflowError? If we want to go that far, we may 
> need to create a special minimal critical method which encapsulates 
> the logic of obtaining Inflater and creating 
> ZipFileInflaterInputStream with it and then put a @ReservedStackAccess 
> annotation on it. Like the following:
>
>     @ReservedStackAccess
>     private ZipFileInflaterInputStream createZipFileInflaterInputStream(
>         ZipFileInputStream zfin, int size
>     ) {
>         Inflater inf = getInflater();
>         ZipFileInflaterInputStream is;
>         try {
>             is = new ZipFileInflaterInputStream(zfin, inf, size,
>                                                 () -> 
> releaseInflater(inf));
>         } catch (OutOfMemoryError e) {
>             releaseInflater(inf);
>             throw e;
>         }
>         try {
>             synchronized (streams) {
>                 streams.add(is);
>             }
>         } catch (OutOfMemoryError e) {
>             try {
>                 is.close();
>             } catch (IOException ignore) {
>                 // not really possible - just making javac happy
>             }
>             throw e;
>         }
>         return is;
>     }
>
>
> What do you think, Sherman?
>

There is reason why those are VirutalMachineError erros. It appears in 
this situation the vm
is in a more critical status than a inflater is getting leaked out. I 
would assume you only catch
these errors if/when the consequence of not catching is more severe and 
the "thing" you are
going to do in catch is not going to trigger more errors. Sure we do 
that here and there in
the libraries code for various reasons, but wonder if here is the place 
that is worth doing that.
That said, if this is a real concern, instead of catching all the 
possible vm erros here and there
to make Cleaner work correctly, a better alternative might be to go back 
to the previous version
to leave the zipfile/deflater cleaner configured/registerd (give up the 
special constructor idea),
so the deflater can get cleaned by itself in case all upstream clean up 
mechanism failed and
the Cleaner mechanism somehow still functions. Opinion?

-Sherman