RFR: 8197812: (ref) Data race in Finalizer

[Peter Levart]

I could even claim that simplifying the if statement in remove() to:

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

makes checking for hasBeenFinalized() in runFinalizer() redundant as it 
would not be possible for runFinalizer() to be called more than once for 
each Finalizer instance because:
- ReferenceQueue never returns the same Reference instance twice or more 
times.
- 'unfinalized' will never point back to the same Finalizer instance for 
the 2nd time, because it always "travels" in the forward direction 
(unfinalized = unfinalized.next).

Regards, Peter

(I rest now).

On 02/14/2018 10:24 AM, Peter Levart wrote:
> Hi,
>
> I may have found a situation described here...
>
> On 02/14/2018 09:47 AM, Peter Levart wrote:
>>
>> 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. 
>
> This is the situation:
>
> unfinalized
>     |
>     v
> Finalizer[1].next --> Finalizer[2].next --> null
> Finalizer[2].prev --> Finalizer[1].prev --> null
>
> runAllFinalizers() is executed and the 1st pass of the for loop takes 
> the Finalizer[1] from the 'unfinalized' pointer and moves the pointer 
> to Finalizer[2]:
>
> unfinalized ----------------
>                            |
>                            v
> Finalizer[1].next --> Finalizer[2].next --> null
> Finalizer[2].prev --> Finalizer[1].prev --> null
>
> ...now finalizer thread kicks-in and takes the Finalizer[2] from the 
> ReferenceQueue and calls remove(). The 1st if statement in remove() 
> would evaluate the condition to true, as 'unfinalized' now points to 
> Finalizer[2], so the body of 1st if would make unfinalized point back 
> to Finalizer[1]:
>
> unfinalized
>     |
>     v
> Finalizer[1].next --> Finalizer[2].next --> null
> Finalizer[2].prev --> Finalizer[1].prev --> null
>
> ...and the remaining remove() code would unlink the Finalizer[2] from 
> the list, resulting in this state of the list:
>
> unfinalized
>     |
>     v
> Finalizer[1].next --> null
> Finalizer[1].prev --> null
>
> This might be seen as a "correct" state, because Finalizer[1] has not 
> been removed from the list yet although it is in the process of being 
> removed because the next thing runAllFinalizers() loop would do is it 
> would call runFinalizer() on it. So the simplified handling in remove():
>
>     if (unfinalized == this) {
>         unfinalized = this.next;
>     }
>
> ...would result in correct behavior too. Even more so because if the 
> 2nd call to runAllFinalizers() was being executed concurrently (I 
> don't know if this is possible though), the Finalizer[1] from above 
> situation would be taken by the concurrent call again and its 
> runFinalizer() executed. runFinalizer() is idempotent so no harm done 
> here, but...
>
> Regards, Peter
>