RFR JDK-8185582, Update Zip implementation to use Cleaner, not finalizers

[Xueming Shen]

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
>>
>