...
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
