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

[Peter Levart]

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
>