Hi Sherman,
On 10/28/2017 09:01 PM, Xueming Shen wrote:
On 10/28/17, 10:47 AM, Peter Levart wrote:
Hi Florian,
On 10/28/17 16:16, Florian Weimer wrote:
* Xueming Shen:
https://bugs.openjdk.java.net/browse/JDK-8187485
http://cr.openjdk.java.net/~sherman/8185582/webrev
In ZipFile:
387 Inflater inf = getInflater();
388 InputStream is = new ZipFileInflaterInputStream(in, inf,
(int)size,
389 () -> releaseInflater(inf));
390 synchronized (streams) {
391 streams.add(is);
392 }
Doesn't this leak the inflater if Cleaner.register() and thus the
ZipFileInflaterInputStream constructor throw?
There is reason why those are VirutalMachineError erros. It appears in
this situation the vm
is in a more critical status than a inflater is getting leaked out. I
would assume you only catch
these errors if/when the consequence of not catching is more severe
and the "thing" you are
going to do in catch is not going to trigger more errors. Sure we do
that here and there in
the libraries code for various reasons, but wonder if here is the
place that is worth doing that.
That said, if this is a real concern, instead of catching all the
possible vm erros here and there
to make Cleaner work correctly, a better alternative might be to go
back to the previous version
to leave the zipfile/deflater cleaner configured/registerd (give up
the special constructor idea),
so the deflater can get cleaned by itself in case all upstream clean
up mechanism failed and
the Cleaner mechanism somehow still functions. Opinion?
-Sherman
I wouldn't go so far as to try to deal with potential
StackOverflowError(s), but for OOMEs, I have the following opinion:
- going back to previous version (give up the special constructor idea)
might not, as you say, be any better. If you just look at the public
Inflater constructor:
121 public Inflater(boolean nowrap) {
122 ZStreamRef ref = new ZStreamRef(init(nowrap), Inflater::end);
123 this.zsRef = ref;
124 this.cleanable = CleanerFactory.cleaner().register(this,
ref);
125 }
...OOME can be thrown after init() and before successful Cleaner
registration in multiple places:
- method reference construction
- ZStreamRef object allocation
- Cleaner.register()
and the probability of that occurring when there is heap exhaustion is
not any smaller.
So I'm more inclined to try to make such code more robust. There will
always be places in such code where some native resource is 1st
allocated, then some logic is executed which allocates objects and
finally Cleaner.register() is attempted. By carefully programming those
steps, a potential native resource leak in case of heap exhaustion can
be prevented. There will usually be a way to back-out (i.e. free the
native resource just allocated) without provoking further trouble. In
the above example, it would be possible to do this:
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;
}
}
An alternative would be to perform all steps that allocate objects
upfront and then, as the final action, allocate the native resource and
"inject" it into the final place:
public Inflater(boolean nowrap) {
ZStreamRef ref = new ZStreamRef(Inflater::end);
this.zsRef = ref;
this.cleanable = CleanerFactory.cleaner().register(this, ref);
ref.initAddress(init(nowrap));
}
Concerning ZipFile, I would make releaseInflater() more robust, because
it is easy:
private void releaseInflater(Inflater inf) {
inf.reset();
try {
synchronized (inflaterCache) {
inflaterCache.add(inf);
}
} catch (OutOfMemoryError e) {
// Inflater.end() does no allocation
inf.end();
throw e;
}
}
For lines 387 ... 392, I would simply do this:
Inflater inf = getInflater();
InputStream is;
try {
is = new ZipFileInflaterInputStream(in, inf, (int)size, () ->
releaseInflater(inf));
} catch (OutOfMemoryError e) {
// calling releaseInflater() is not sensible here, because it
allocates and will
// probably result in Inflater.end() anyway...
inf.end();
throw e;
}
try {
synchronized (streams) {
streams.add(is);
}
} catch (OutOfMemoryError e) {
// ZipFileInflaterInputStream.close() does not allocate,
IOException is never thrown
try { is.close(); } catch (IOException ignore) {}
throw e;
}
Simply saying that "vm is in a more critical status than a inflater is
getting leaked out" is, in my opinion, covering the problem under the
rug. The VM is not in critical state - the program is. VM is robust
enough to recover from OOMEs. The program might be in critical status
(i.e. in inconsistent state) partly because of not accounting for such
OOME situations. By taking care of them, the program has a better chance
of recovering from such situation(s).
Handling native resources is one place where I think it is beneficial to
complicate things in order to make native resource leaks (im/less
)possible. The other such place is synchronization primitives. We must
admit that finalization does have this benefit that it makes it hard to
construct code that would allocate the native resource before cleanup
registration (which is performed as part of Object.<init>, while logic
to allocate native resource is usually invoked from subclass
constructor). 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.
Regards, Peter
