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

[Peter Levart]

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?

Regards, Peter