On 10/28/17, 7:16 AM, Florian Weimer wrote:
* Xueming Shen:
I removed the ln#657-#663, and updated the @apiNote in deflter, inflater
and zipfile accordingly, mainly combined your comment and the approach
for the fis/fo. they are "simpler" and straightforward now, at least for me.
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?
Hi Florian, thanks for reviewing.
The answer is "yes", if Cleaner.register() fails and throws a runtime
exception ( internal
error probably?) somewhere inside its implementation. I'm now an expert
on Cleaner's
implementation, but the "register" operation looks straightforward,
creating the phantom
ref object and "inserting" the newly created phantom ref into the queue,
which appears
unlikely to fail easily. The "assumption" of taking the approach to move
away from finalize()
to cleaner is that "Cleaner.register()" should work as reliable as
finalizer. But in situation it
really fails, I would assume there is really nothing can be done to save
the world here. Similar
to the situation like "free()" fails (for the "calloc()/free()" pair)
and the memory to be
deallocated might be left "un-freed"?
In the latest version (this webrev) the inflater's default "cleaner" is
turned off via a package
private constructor, with the assumption that it is guaranteed that the
inflater will always be
putback into the "inflaterCache" by the corresponding ZFIIS cleaner and
all inflaters inside
"inflaterCache" is guaranteed to be cleaned up via the ZipFile cleaner.
So yes arguably we are
less safe by removing that (if this is the concern) But if
Cleaner.register() here can fail, there
is no guarantee inflater's default Cleaner.register() will not fail.
It might be possible to try-catch to expect Cleaner.register might fail,
but I doubt it's really
necessary here and it is the recommended usage of Cleaner?
Thanks!
-Sherman
