Hi Peter,
Sorry, I might not understand your use scenario correctly. Let me try :-)
If clean() is invoked via Deflater.end() first, my reading of the
Cleaner code suggests that
the clean()->ZStreamRef.run() is run by the thread calling
deflater.end() directly, not
the Cleaner thread. In this scenario, since we are still inside the
synchronized(zsRef)
block, all other "zsRef" sensitive deflater methods should be blocked by
the same
synchronzied(zeRef) ? I would assume we don't have problem in this use
scenario?
567 public void end() {
568 synchronized (zsRef) {
569 cleanable.clean();
570 buf = null;
571 }
572 }
If some other method is invoked first, for example the
setDictionary(...) as in your
sample, any direct invocation of delfater.end() from other thread should
be blocked
because of "synchronized(zsRef)". So I would assume this is not a
concern. It seems
the concern is that the "Deflater.this" object itself is no longer
needed and becomes
unreachable after line#261, therefore this deflater becomes
phantom-reachable and
the ZStreamRef.run() is called by the Cleaner thread, then we have a
race condition
between the thread calls the setDictionary() and the Cleaner thread? I
guess I might
have a wrong assumption here, is it true that if someone/thread is calling
deflater.setDictionary(), that someone/thread should still hold a
reference to the deflater
and therefore prevent it from becoming phantom-reachable? Or you are
saying it
is possible under the hotspot optimization the deflater.setDictionary()
(and its ensureOpen()
included) is being inline-ed and therefore there is no more reference to
that deflater
even we are still inside that deflater.setDictionary(), so that "after
ine#261" scenario
becomes possible?
Thanks,
Sherman
On 9/27/17, 2:31 AM, 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
