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

[Peter Levart]

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