RFR: 8197812: (ref) Data race in Finalizer

[Peter Levart]

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