RFR JDK-8185582, Update Zip implementation to use Cleaner, not finalizers
Xueming Shen
xueming.shen at oracle.com
Mon Oct 30 18:45:46 UTC 2017
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
More information about the core-libs-dev
mailing list