Hi David,
On 09/27/2017 11:52 AM, David Holmes wrote:
Hi Peter,
I thought Cleaner was supposed to avoid all these unpredictable
reachability races? Otherwise why switch from using a finalizer ??
Unfortunately, there is no hidden magic in Cleaner. It is better than
finalize() mostly because the clean-up function may be invoked by the
user, which also de-registers it, allowing GC to collect it rather than
leaving it to reference-processing pipeline to process it for no reason.
And of course, it guarantees that the tracked referent can not be
resurrected as a result of cleanup code execution. What remains
unchanged with Cleaner is the point in program execution when the
tracked referent is determined to be phantom-reachable (finalizable) and
therefore its associated PhantomReference(s) eligible for processing
(finalize() invoked).
Regards, Peter
David
On 27/09/2017 7:31 PM, Peter Levart wrote:
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
