Hi Sherman,
At first I checked the Deflater/Inflater changes. I'll come back with
ZipFile later.
I think the code is mostly correct, but I have a concern. If clean() is
invoked via Deflater.end(), then the Deflater instance is still alive
and synchronization is necessary as other threads might be concurrently
invoking other methods. If clean() is invoked via Cleaner thread, then
Deflater instance is already phantom reachable and no thread may be
accessing it or be in the middle of executing any of its instance
methods. How is this guaranteed? Critical Deflater methods synchronize
on the zsRef instance, so at least at the beginning of the synchronized
block, the Deflater instance is strongly reachable. Take for example the
following Deflater instance method:
254 public void setDictionary(byte[] b, int off, int len) {
255 if (b == null) {
256 throw new NullPointerException();
257 }
258 if (off < 0 || len < 0 || off > b.length - len) {
259 throw new ArrayIndexOutOfBoundsException();
260 }
261 synchronized (zsRef) {
262 ensureOpen();
263 setDictionary(zsRef.address(), b, off, len);
264 }
265 }
Up to a point where 'this' is dereferenced to obtain the 'zsRef' value
(line 261), the Deflater instance is reachable. But after that, even
ensureOpen() may be inlined and 'this' is not needed any more. After
that point, obtaining zsRef.address() and calling setDictionaly on the
obtained address may be racing with Cleaner thread invoking
ZStreamRef.run():
49 public void run() {
50 long addr = address;
51 address = 0;
52 if (addr != 0) {
53 end.accept(addr);
54 }
55 }
Possible program execution is therefore:
Thread1:
zsRef.address() - returning non-zero address
Thread2:
ZStreamRef.run() - invoking Deflater.end(address) with non-zero address
via the passed-in lambda.
Thread1:
setDictionary(<non-zero address from before>, b, off, len)
To fix this, you could sprinkle Reference.reachabilityFence(this) at
appropriate places in Deflater, but it is simpler to just add
synchronized modifier to ZStreamRef.run() method. There's no danger of
deadlock(s) since Cleaner ensures the run() method is invoked at most once.
The same reasoning applies to Inflater to as code is similar.
A nit (Deflater and similar to Inflater):
187 ZStreamRef ref = new ZStreamRef(init(level,
DEFAULT_STRATEGY, nowrap),
188 addr -> end(addr));
You could use a method reference there.
Regards, Peter
On 09/27/2017 08:35 AM, Xueming Shen wrote:
Hi,
Please help review the change for JDK-8185582.
issue: https://bugs.openjdk.java.net/browse/JDK-8185582
webrev: http://cr.openjdk.java.net/~sherman/8185582/webrev/
csr: https://bugs.openjdk.java.net/browse/JDK-8187485
The proposed change here is to replace the finalize() cleanup
mechanism with
the newly introduced java.lang.ref.Cleaner for java.util.zip package
(Deflater,
Inflater, ZipFile and
ZipFile.ZipFileInputStrean/ZipFileInflaterInputStream).
Since it's an "incompatible" change, both behavioral and source
incompatibility,
to remove the public finalize() from these classes, a corresponding
CSR is also
created and need review as well.
Thanks,
Sherman
