Re: RFR: 8197812: (ref) Data race in Finalizer

[Martin Buchholz]

Thanks, Mandy, changeset pushed.  We are in agreement.

On Thu, Feb 15, 2018 at 7:31 PM, mandy chung  wrote:

>
>
> On 2/15/18 7:13 PM, Martin Buchholz wrote:
>
>
>
> On Thu, Feb 15, 2018 at 3:20 PM, mandy chung 
> wrote:
>
>>
>> I have posted RFR to remove runFinalizersOnExit.   I think your patch
>> would become cleaner as you may keep the remove method or inline in
>> runFinalizer method as your original patch has.
>>
>> Do you want to wait until JDK-8198249?  or proceed this patch without
>> waiting?
>>
>
> I'd like to commit this patch now, in part because we're actually
> backporting this as a local patch to make race detection easier.
>
> Regarding remove(), I think it's actually cleaner inline, but if it's kept
> a separate method, then make it return a boolean, as with Collection remove
> methods.
>
>
> http://cr.openjdk.java.net/~martin/webrevs/jdk/Finalizer-data-race/
>
> I reviewed this version.  It looks okay.
>
> With the removal of runFinalizersOnExit, I will inline the deregistration
> part as part of runFinalizers after I rebase with your change.  Hope that's
> okay with you.
>
> Mandy
>

Re: RFR: 8197812: (ref) Data race in Finalizer

[mandy chung]

On 2/15/18 7:13 PM, Martin Buchholz wrote:



On Thu, Feb 15, 2018 at 3:20 PM, mandy chung > wrote:



I have posted RFR to remove runFinalizersOnExit.   I think your
patch would become cleaner as you may keep the remove method or
inline in runFinalizer method as your original patch has.

Do you want to wait until JDK-8198249?  or proceed this patch
without waiting?


I'd like to commit this patch now, in part because we're actually 
backporting this as a local patch to make race detection easier.


Regarding remove(), I think it's actually cleaner inline, but if it's 
kept a separate method, then make it return a boolean, as with 
Collection remove methods.




http://cr.openjdk.java.net/~martin/webrevs/jdk/Finalizer-data-race/

I reviewed this version.  It looks okay.

With the removal of runFinalizersOnExit, I will inline the 
deregistration part as part of runFinalizers after I rebase with your 
change.  Hope that's okay with you.


Mandy

Re: RFR: 8197812: (ref) Data race in Finalizer

[Martin Buchholz]

On Thu, Feb 15, 2018 at 3:20 PM, mandy chung  wrote:

>
> I have posted RFR to remove runFinalizersOnExit.   I think your patch
> would become cleaner as you may keep the remove method or inline in
> runFinalizer method as your original patch has.
>
> Do you want to wait until JDK-8198249?  or proceed this patch without
> waiting?
>

I'd like to commit this patch now, in part because we're actually
backporting this as a local patch to make race detection easier.

Regarding remove(), I think it's actually cleaner inline, but if it's kept
a separate method, then make it return a boolean, as with Collection remove
methods.

Re: RFR: 8197812: (ref) Data race in Finalizer

[mandy chung]

Hi Martin,

On 2/14/18 9:47 PM, Martin Buchholz wrote:
Embarrassed by my failure to take runFinalizersOnExit into account, I 
tried to redo the patch to be actually correct.
http://cr.openjdk.java.net/~martin/webrevs/jdk/Finalizer-data-race/ 

even though we may succeed in making runFinalizersOnExit go away, and 
I'm not planning to submit any time soon.
The runFinalizersOnExit case needs different mechanics for removing 
elements from "unfinalized".
The simple invariant is that you need to run the finalizer whenever an 
element is unlinked from unfinalized.




I have posted RFR to remove runFinalizersOnExit.   I think your patch 
would become cleaner as you may keep the remove method or inline in 
runFinalizer method as your original patch has.


Do you want to wait until JDK-8198249?  or proceed this patch without 
waiting?


Mandy
[1] 
http://mail.openjdk.java.net/pipermail/core-libs-dev/2018-February/051518.html

Re: RFR: 8197812: (ref) Data race in Finalizer

[Martin Buchholz]

On Wed, Feb 14, 2018 at 11:47 PM, Peter Levart 
wrote:

>
> Although not strictly necessary for correctness, for the time being, it
> would be nice to be consistent in marking the Finalizer object "already
> finalized" in both places. Either set both next and prev to this or next to
> this and prev to null.
>
>
OK, now only next is ever self-linked.

this.prev = null;
this.next = this;   // mark as finalized

Re: RFR: 8197812: (ref) Data race in Finalizer

[Peter Levart]

Hi Martin,

On 02/15/2018 06:47 AM, Martin Buchholz wrote:

Embarrassed by my failure to take runFinalizersOnExit into account, I tried
to redo the patch to be actually correct.


I think that your patch was correct the first time too, but in view of 
future removing the need for "pollFirst" from unfinalized list, the 
revised patch separates this logic entirely into the to-be-removed 
method so it is preferable.



  http://cr.openjdk.java.net/~martin/webrevs/jdk/Finalizer-data-race/
even though we may succeed in making runFinalizersOnExit go away, and I'm
not planning to submit any time soon.
The runFinalizersOnExit case needs different mechanics for removing
elements from "unfinalized".
The simple invariant is that you need to run the finalizer whenever an
element is unlinked from unfinalized.



...and the check for "already been finalized" needs to be performed only 
in deregisterAndRunFinalizer as you did in new patch. Once 
runAllFinalizers is removed, this check and marking code could be 
removed too.


Although not strictly necessary for correctness, for the time being, it 
would be nice to be consistent in marking the Finalizer object "already 
finalized" in both places. Either set both next and prev to this or next 
to this and prev to null.


Regards, Peter

Re: RFR: 8197812: (ref) Data race in Finalizer

[Martin Buchholz]

Embarrassed by my failure to take runFinalizersOnExit into account, I tried
to redo the patch to be actually correct.
 http://cr.openjdk.java.net/~martin/webrevs/jdk/Finalizer-data-race/
even though we may succeed in making runFinalizersOnExit go away, and I'm
not planning to submit any time soon.
The runFinalizersOnExit case needs different mechanics for removing
elements from "unfinalized".
The simple invariant is that you need to run the finalizer whenever an
element is unlinked from unfinalized.

Re: RFR: 8197812: (ref) Data race in Finalizer

[mandy chung]

On 2/14/18 1:54 PM, Martin Buchholz wrote:
Indeed I had suppressed all memory of runFinalizersOnExit. (Sorry 
about that.)

I support removing it in jdk11.


Great.


Mandy, would you like to file the CSR?


Yes I plan to do that for:
    https://bugs.openjdk.java.net/browse/JDK-4240589

Mandy


On Wed, Feb 14, 2018 at 1:15 PM, mandy chung > wrote:




On 2/14/18 1:58 AM, Peter Levart wrote:


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.


runAllFinalizers is invoked during shutdown when
System.runFinalizersOnExit has been called.

I have been wanting to remove System::runFinalizersOnExit [1]
which is the culprit of causing this complicated handling.  
Probably time to remove it in 11?

Mandy

[1]

http://mail.openjdk.java.net/pipermail/core-libs-dev/2015-January/031041.html

Re: RFR: 8197812: (ref) Data race in Finalizer

[Martin Buchholz]

Indeed I had suppressed all memory of runFinalizersOnExit.  (Sorry about
that.)
I support removing it in jdk11.
Mandy, would you like to file the CSR?

On Wed, Feb 14, 2018 at 1:15 PM, mandy chung  wrote:

>
>
> On 2/14/18 1:58 AM, Peter Levart wrote:
>
>
> 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.
>
>
> runAllFinalizers is invoked during shutdown when
> System.runFinalizersOnExit has been called.
>
> I have been wanting to remove System::runFinalizersOnExit [1] which is the
> culprit of causing this complicated handling.   Probably time to remove it
> in 11?
>
> Mandy
>
> [1] http://mail.openjdk.java.net/pipermail/core-libs-dev/2015-
> January/031041.html
>

Re: RFR: 8197812: (ref) Data race in Finalizer

[mandy chung]

On 2/14/18 1:58 AM, Peter Levart wrote:


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.


runAllFinalizers is invoked during shutdown when 
System.runFinalizersOnExit has been called.


I have been wanting to remove System::runFinalizersOnExit [1] which is 
the culprit of causing this complicated handling.   Probably time to 
remove it in 11?


Mandy

[1] 
http://mail.openjdk.java.net/pipermail/core-libs-dev/2015-January/031041.html

Re: RFR: 8197812: (ref) Data race in Finalizer

[Peter Levart]

On 02/14/2018 11:49 AM, Peter Levart wrote:

Hi Martin,

On 02/14/2018 10:58 AM, Peter Levart wrote:


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.


... but this could be "fixed" if the taking of next Finalizer from 
'unfinalized' list and removing it from the same list was a single 
atomic operation. What do you say of the following further 
simplification:


http://cr.openjdk.java.net/~plevart/jdk-dev/8197812_Data_race_in_Finalizer/webrev.01/ 



Even that has a flaw. Running the next unfinalized Finalizer from 
runAllFinalizers() does not prevent the same Finalizer to be returned 
from the ReferenceQueue. So the check must remain in place.


Sorry for this long monologue. I truly rest now.

Regards, Peter

Re: RFR: 8197812: (ref) Data race in Finalizer

[Peter Levart]

Hi Martin,

On 02/14/2018 10:58 AM, Peter Levart wrote:


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.


... but this could be "fixed" if the taking of next Finalizer from 
'unfinalized' list and removing it from the same list was a single 
atomic operation. What do you say of the following further simplification:


http://cr.openjdk.java.net/~plevart/jdk-dev/8197812_Data_race_in_Finalizer/webrev.01/

Regards, Peter



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).

Re: 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

Re: 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

Re: 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

Re: 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

Re: RFR: 8197812: (ref) Data race in Finalizer

[Peter Levart]

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

Re: RFR: 8197812: (ref) Data race in Finalizer

[Peter Levart]

Hi Martin,

On 02/14/18 02:30, Martin Buchholz wrote:

Unlike most fixes to data races, this one should benefit performance.

http://cr.openjdk.java.net/~martin/webrevs/jdk/Finalizer-data-race/
https://bugs.openjdk.java.net/browse/JDK-8197812


Nice catch (for java-tsan tool)!

Although I think this race is benign. The code that synchronizes on 
'this' is the following:


  46 private boolean hasBeenFinalized() {
  47 return (next == this);
  48 }

And the only place where 'next' is set to 'this' is in remove() and 
remove is called from synchronized (this) block too. So 
hasBeenFinalized() may see any or no writes of 'next' performed out of 
synchronized (this), but they are never writes of value 'this', so the 
outcome is always 'false' in this case. The only write to 'next' with 
value of 'this' happens while synchronized (this) and synchronized 
(lock) at the same time so this write synchronizes with the read of 
'next' in hasBeenFinalized() as well as with all other writes of 'next'.


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?


Regards, Peter

RFR: 8197812: (ref) Data race in Finalizer

[Martin Buchholz]

Unlike most fixes to data races, this one should benefit performance.

http://cr.openjdk.java.net/~martin/webrevs/jdk/Finalizer-data-race/
https://bugs.openjdk.java.net/browse/JDK-8197812