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