Phantom Referencesvs finalizers (was: RFR JDK-8185582, Update Zip implementation to use Cleaner, not finalizers)

Wed Sep 27 17:55:13 UTC 2017

"And of course, it guarantees that the tracked referent can not be
resurrected as a result of cleanup code execution."

True, but it seems to me that the real property you want is "it guarantees
that the tracked referent can not be resurrected". Full stop. Actually, the
property I really need is: "The referent of an enqueued PhantomReference
cannot subsequently be accessed."

Unfortunately, neither of those last two appear to be true due to the
weakly specified behavior of JNI WeakGlobalRefs. The spec says:

"Interactions between weak global references and PhantomReferences are
undefined. In particular, implementations of a Java VM may (or may not)
process weak global references after PhantomReferences, and it may (or may
not) be possible to use weak global references to hold on to objects which
are also referred to by PhantomReference objects. This undefined use of
weak global references should be avoided."

This allows a WeakGlobalRef to be converted to a strong JNI ref after a
PhantomReference to the same object has been enqueued, thus resurrecting
the referent, and allowing access.

The advice in the last sentence seems to be both essential and completely
impractical, since the WeakGlobalRef may point to another object, which may
then indirectly point ot the PhantomReference referent. Hence you can't
prevent this interaction without global, whole program knowledge of which
objects may indirectly reference PhantomReference referents. This is
essentially the same problem we had with (unordered) finalization, only now
confined to programs that use WeakGlobalRefs.

Am I reading this correctly? Is it intended? It seems like it should be
fixable by giving WeakGlobalRefs essentially WeakReference behavior, as the
name implies?

On Wed, Sep 27, 2017 at 3:36 AM, Peter Levart <peter.levart at gmail.com>
wrote:

> 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/Zip
>>>> FileInflaterInputStream).
>>>>
>>>> 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
>>>>
>>>>
>>>
>