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

[Xueming Shen]

...
   long t0 = System.nanoTime();
   this.zsrc = Source.get(file, (mode & OPEN_DELETE) != 0);
   this.streams = Collections.newSetFromMap(new WeakHashMap<>
   this.inflaterCache = new ArrayDeque<>();
   this.cleanable = CleanerFactory.cleaner().register(this,
           new Releaser(this.streams, this.inflaterCache, this.zsrc));
...

Now look at the "cleaner" for ZipFile. Obviously the
new Releaser(...) might trigger OOME ... So we have to
do something similar and move this piece to the top of the ZipFile
constructor before any resource gets allocated, before we have the
"zsrc"... yes, we have to inject the "zsrc" later. As, the zsrc will not
get released if we allocate it before register.

It appears we have a general usage issue here with the Cleaner? if the
unexpected throwable needs to be taken care of. The resource needed
to be released must be obtained after you have the cleaner created and
successfully registered. Otherwise it might be leaked out if the "cleaner
creation and registration" throws an unexpected throwable. As I suggested
in my early email, try/catch might not simply work here as well, as an
unexpected throwable from CleanerFactory.cleaner().register(...) really
tells nothing about the status of the cleaner being registered, so the
"cleaner" implementation will have to be crafted specially to take care of
this situation.

Sherman

On 10/30/2017 11:45 AM, 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.
>
> 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.
>
>      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 "safe" approach seems to be the
> "alternative".
>
> 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. 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, 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. 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.
>
> 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)
>
> thanks,
> Sherman
>
>
>
>