RFR: 8197812: (ref) Data race in Finalizer

[Peter Levart]

Hi Again,

While studying the code of Finalizer I found a strange discrepancy 
between handling of 'unfinalized' field during processing of 
Finalizer(s) taken from ReferenceQueue and taken from the 'unfinalized' 
pointer itself (as in runAllFinalizers()). The remove() method is as 
follows:

     private void remove() {
         synchronized (lock) {
             if (unfinalized == this) {
                 if (this.next != null) {
                     unfinalized = this.next;
                 } else {
                     unfinalized = this.prev;
                 }
             }
             if (this.next != null) {
                 this.next.prev = this.prev;
             }
             if (this.prev != null) {
                 this.prev.next = this.next;
             }
             this.next = this;   /* Indicates that this has been 
finalized */
             this.prev = this;
         }
     }

When Finalizer(s) are taken from ReferenceQueue, they are processed in 
arbitrary order, so once in a while it can happen that the Finalizer at 
the head of the list (pointed to by 'unfinalized') is processed this 
way. The body of the 1st if statement in above method is executed in 
such situation:

             if (unfinalized == this) {
                 if (this.next != null) {
                     unfinalized = this.next;
                 } else {
                     unfinalized = this.prev;
                 }
             }

But I can't figure out a situation when this.next would be null while 
this.prev would be non-null. In other words, when 'unfinalized' would 
not point to the head of the list. For comparison, when Finalizer(s) are 
processed from the 'unfinalized' pointer directly (as in 
runAllFinalizers()), the following is executed:

                 for (;;) {
                     Finalizer f;
                     synchronized (lock) {
                         f = unfinalized;
                         if (f == null) break;
                         unfinalized = f.next;
                     }
                     f.runFinalizer(jla);
                 }

... so it is assumed that the 'unfinalized' always points to the head. 
If this was not true, not all Finalizer(s) would be processed.

So what I'm asking is whether this simplified if statement in remove() 
would be equivalent:

             if (unfinalized == this) {
                 unfinalized = this.next;
             }

Or am I missing some situation?

Regards, Peter

On 02/14/2018 09:19 AM, Peter Levart wrote:
> Hi Martin,
>
> On 02/14/2018 07:58 AM, Peter Levart wrote:
>> It may be that the intent was to refrain from using the shared 'lock' 
>> lock for the 2nd and subsequent calls to runFinalizer() and only use 
>> the more fine-grained 'this' lock in this case?
>>
>> If someone was able to call runFinalizer() on the same instance in a 
>> loop he could prevent or slow-down normal processing of other 
>> Finalizer(s) if the shared 'lock' was always employed. What do you 
>> think? 
>
> I checked all uses of runFinalizer() and I don't think it can be 
> called more than twice for the same Finalizer instance (for example if 
> there is a race between runAllFinalizers() and processing of 
> Finalizers taken from the ReferenceQueue). So your patch is a nice 
> simplification.
>
> Regards, Peter
>