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
