RFR: 8197812: (ref) Data race in Finalizer

[Peter Levart]

I take back this claim. Of course the the following race is possible:

- Thread1: calls runAllFinalizers and takes a Finalizer from 
'unprocessed' list.
- Thread2: takee the same Finalizer instance from ReferenceQueue and 
calls runFinalizer()
- Thread1: calls runFinalizer() with the same instance for the 2nd time now.

Regards, Peter

On 02/14/2018 10:39 AM, Peter Levart wrote:
> 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
>>
>