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
