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

[David Holmes]

Hi Peter,

I thought Cleaner was supposed to avoid all these unpredictable 
reachability races? Otherwise why switch from using a finalizer ??

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