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?
Thanks for being alert. I think that all that can be thrown between
getInfalter() call and streams.add() successfully returning is
OutOfMemoryError or StackOverflowError. OOME can be thrown anywhere,
not only as a consequence of Cleaner.register(). I see following
potential places where allocation may be taking place:
- constructing the lambda instance (line 389)
- allocating ZipFileInflaterInputStream object
- multiple places in ZipFileInflaterInputStream.<init> and
InflaterInputStream.<init> (including but not limited to
Cleaner.register())
- streams.add() (line 391)
Can we do anything about it? Let's see. For example:
Inflater inf = getInflater();
InputStream is;
try {
is = new ZipFileInflaterInputStream(in, inf, (int)size, () ->
releaseInflater(inf));
} catch (OutOfMemoryError e) {
releaseInflater(inf);
throw e;
}
try {
synchronized (streams) {
streams.add(is);
}
} catch (OutOfMemoryError e) {
try { is.close(); } catch (IOException ignore) {}
}
...but, even releaseInflater(inf) may throw OOME (in line 470):
467 private void releaseInflater(Inflater inf) {
468 inf.reset();
469 synchronized (inflaterCache) {
470 inflaterCache.add(inf);
471 }
472 }
So we may change it to the following, which would make it more robust
for other uses too:
private void releaseInflater(Inflater inf) {
inf.reset();
try {
synchronized (inflaterCache) {
inflaterCache.add(inf);
}
} catch (OutOfMemoryError e) {
inf.end();
throw e;
}
}
...and that's it for OOME, hopefully.
But what about StackOverflowError? If we want to go that far, we may
need to create a special minimal critical method which encapsulates
the logic of obtaining Inflater and creating
ZipFileInflaterInputStream with it and then put a @ReservedStackAccess
annotation on it. Like the following:
@ReservedStackAccess
private ZipFileInflaterInputStream createZipFileInflaterInputStream(
ZipFileInputStream zfin, int size
) {
Inflater inf = getInflater();
ZipFileInflaterInputStream is;
try {
is = new ZipFileInflaterInputStream(zfin, inf, size,
() ->
releaseInflater(inf));
} catch (OutOfMemoryError e) {
releaseInflater(inf);
throw e;
}
try {
synchronized (streams) {
streams.add(is);
}
} catch (OutOfMemoryError e) {
try {
is.close();
} catch (IOException ignore) {
// not really possible - just making javac happy
}
throw e;
}
return is;
}
What do you think, Sherman?
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
