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

[Peter Levart]

Hi Sherman,

On 10/28/2017 09:01 PM, Xueming Shen wrote:
> 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?
>>
>>
>
> 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
>

I wouldn't go so far as to try to deal with potential 
StackOverflowError(s), but for OOMEs, I have the following opinion:

- going back to previous version (give up the special constructor idea) 
might not, as you say, be any better. If you just look at the public 
Inflater constructor:

  121     public Inflater(boolean nowrap) {
  122         ZStreamRef ref = new ZStreamRef(init(nowrap), Inflater::end);
  123         this.zsRef = ref;
  124         this.cleanable = CleanerFactory.cleaner().register(this, 
ref);
  125     }

...OOME can be thrown after init() and before successful Cleaner 
registration in multiple places:

- method reference construction
- ZStreamRef object allocation
- Cleaner.register()

and the probability of that occurring when there is heap exhaustion is 
not any smaller.

So I'm more inclined to try to make such code more robust. There will 
always be places in such code where some native resource is 1st 
allocated, then some logic is executed which allocates objects and 
finally Cleaner.register() is attempted. By carefully programming those 
steps, a potential native resource leak in case of heap exhaustion can 
be prevented. There will usually be a way to back-out (i.e. free the 
native resource just allocated) without provoking further trouble. In 
the above example, it would be possible to do this:

      public Inflater(boolean nowrap) {
         long address = init(nowrap);
         try {
             ZStreamRef ref = new ZStreamRef(address, Inflater::end);
             this.zsRef = ref;
             this.cleanable = CleanerFactory.cleaner().register(this, ref);
         } catch (OutOfMemoryError oome) {
             end(address);
             throw oome;
         }
     }

An alternative would be to perform all steps that allocate objects 
upfront and then, as the final action, allocate the native resource and 
"inject" it into the final place:

     public Inflater(boolean nowrap) {
         ZStreamRef ref = new ZStreamRef(Inflater::end);
         this.zsRef = ref;
         this.cleanable = CleanerFactory.cleaner().register(this, ref);
         ref.initAddress(init(nowrap));
     }


Concerning ZipFile, I would make releaseInflater() more robust, because 
it is easy:

  private void releaseInflater(Inflater inf) {
    inf.reset();
    try {
        synchronized (inflaterCache) {
            inflaterCache.add(inf);
        }
    } catch (OutOfMemoryError e) {
        // Inflater.end() does no allocation
        inf.end();
        throw e;
    }
}

For lines 387 ... 392, I would simply do this:

Inflater inf = getInflater();
InputStream is;

try {
     is = new ZipFileInflaterInputStream(in, inf, (int)size, () -> 
releaseInflater(inf));
} catch (OutOfMemoryError e) {
     // calling releaseInflater() is not sensible here, because it 
allocates and will
     // probably result in Inflater.end() anyway...
     inf.end();
     throw e;
}

try {
     synchronized (streams) {
         streams.add(is);
     }
} catch (OutOfMemoryError e) {
     // ZipFileInflaterInputStream.close() does not allocate, 
IOException is never thrown
     try { is.close(); } catch (IOException ignore) {}
     throw e;
}


Simply saying that "vm is in a more critical status than a inflater is 
getting leaked out" is, in my opinion, covering the problem under the 
rug. The VM is not in critical state - the program is. VM is robust 
enough to recover from OOMEs. The program might be in critical status 
(i.e. in inconsistent state) partly because of not accounting for such 
OOME situations. By taking care of them, the program has a better chance 
of recovering from such situation(s).

Handling native resources is one place where I think it is beneficial to 
complicate things in order to make native resource leaks (im/less 
)possible. The other such place is synchronization primitives. We must 
admit that finalization does have this benefit that it makes it hard to 
construct code that would allocate the native resource before cleanup 
registration (which is performed as part of Object.<init>, while logic 
to allocate native resource is usually invoked from subclass 
constructor). To achieve the same robustness with Cleaner API, one has 
to be careful to either perform registration upfront and then allocate 
native resource or arrange for back-out in case of trouble.

Regards, Peter