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

[Peter Levart]

Hi Sherman,

On 10/30/17 19:45, Xueming Shen wrote:
> Peter,
>
> Given we have to put in those "make it more robust" code in ZipFile to 
> make cleaner
> work correctly with the zipfile/inflater in those vm error 
> circumstances I would assume
> it is a wrong design decision to have the short-cut Inflater 
> constructor to try to save
> some runtime circle for ZipFile/Inflater/cleaner. If the only purpose 
> of those code is to
> deal with the rare vm error situation in which we can't call 
> inflater.end() normally, then
> arguably this is the main reason we have the cleaner mechanism at 
> first place, and
> we probably should just let the cleaner to do the job (clean the 
> resource when the
> normal cleanup/release path does not work), instead of having yet 
> another mechanism
> to replace it, and with more code to workaround the possible rare 
> circumstances.

Ok, but in that case the Cleaner registration in Inflater should be 
reliable and not fail in the same circumstances.

>
> Yes, if the vm error is a concern, the usage/implementation in 
> Deflater/Inflater/ZStreamRef
> has the similar problem. Potentially the try/catch approach might have 
> issues. Arguably
> the OOME might come from "register", and in theory there is no way to 
> know whether
> or not the OOME is triggered before the "cleaner" has been 
> successfully put in the Queue
> already or after If later, the cleaner might still be invoked later by 
> the Cleaner to try to
> release the memory one more time.

The Cleaner.register() can only fail with OOME and only because the 
jdk.internal.ref.CleanerImpl.PhantomCleanableRef object allocation 
fails. This *is the only* allocation performed by Cleaner.register(). If 
PhantomCleanableRef object allocation fails (a PhantomReference 
subclass), no PhantomReference instance is created and therefore can not 
be discovered by GC and consequently no cleanup happens.

>
>      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;
>         }
>     }
>
> A normal return from register tells us everything is fine, the cleaner 
> is registered
> and it will perform as expected, but an unexpected 
> RuntimeException/Error from
> register really tells us nothing for now. 

The only RuntimeException possible from Cleaner.register() is a 
NullPointerException because of null operands and the only possible 
Error is OOME which in both cases tells us that registration did not 
happen. As said, Cleaner.register() allocates a single instance - an 
instance of jdk.internal.ref.CleanerImpl.PhantomCleanableRef. Everything 
else is just "dancing with links".

> The only "safe" approach seems to be the
> "alternative".

I agree that it is a more direct and simple approach to just reorder the 
operations, rather than arrange for back-out, but I think it is equally 
safe. Might be good to put some comments just before the init() to 
explain why it is done last in Inflater constructor (same for Deflater)?

>
> As you suggested "..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." It appears this might to 
> be a very general
> usage issue of the "cleaner" mechanism. 

Yes. It appears so. But OTOH it is something that is easily understood 
and might be beneficial to document in the Cleaner javadoc.

> Now other than the "cleaning code should
> not have object reference the object being registered" restriction, it 
> might be dirsired
> to have another suggestion/warning somewhere on the "order" of 
> register the cleaner
> and the creation of the resource to be cleaned, 

Right. To mimic the finalization registration, it might be a good to 
encourage the following coding pattern (using Inflater/ZStreamRef just 
as an example, not suggesting to do that here unless you like it):

class ZStreamRef implements Runnable {

     private final LongConsumer end;
     private volatile long address;
     final Cleaner.Cleanable cleanable; // move cleanable from 
Inflater/Deflater to here

     ZStreamRef (Object reference, LongSupplier init, LongConsumer end) {
         // perform registration as 1st thing in constructor
         cleanable = CleanerFactory.cleaner().register(reference, this);
         this.end = end;
         this.address = init.getAsLong();
     }

     long address() {
         return address;
     }

     public synchronized void run() {
         long addr = address;
         address = 0;
         if (addr != 0) {
             end.accept(addr);
         }
     }
}


// ... and then in Inflater / Deflater:

     public Inflater(boolean nowrap) {
         this.zsRef = new ZStreamRef(this, () -> init(nowrap), 
Inflater::end);
     }

     public Deflater(int level, boolean nowrap) {
         this.level = level;
         this.strategy = DEFAULT_STRATEGY;
         this.zsRef = new ZStreamRef(
                                     this,
                                     () -> init(level, DEFAULT_STRATEGY, 
nowrap),
                                     Deflater::end);
     }

     public void end() {
         synchronized (zsRef) {
             zsRef.cleanable.clean();
             buf = null;
         }
     }


> and probably some guarantee that
> the "state" of the registered cleaner, still functional or thrown 
> away, when the
> unexpected happens, such as a VM Error gets thrown during registration. 

I'm pretty sure it is guaranteed that Cleaner.register() throwing OOME 
means that no registration happened.

> Which
> reminds me the question asked early regarding other Cleaner use 
> scenario. It
> appears, based on my experience of using Cleaner in this case, even 
> the finalize()
> mechanism has "lots" of issues in its implementation, it provides a 
> "nice/clean/simple"
> API to the "clean up the resource when not used" problem, while the 
> Cleaner API
> appears to have lots of restriction to use it correctly.

It is not trivial to use, but it has benefits that finalization lacks. 
Like on-demand cleanup with de-registration.

>
> Webrev has been updated to (1) remove the special Inflater() (2) 
> "allocate the
> resource and inject it later" for in/deflater.
>
> http://cr.openjdk.java.net/~sherman/8185582/webrev
> (the preview webrev has been rename to webrev.04)

The Inflater and Deflater now look fine (except you don't have to check 
for cleanable != null any more in Inflater.end()).

But what shall we do with ZipFile?

>
> thanks,
> Sherman
>

Regards, Peter