On 6/19/13 5:05 AM, David Holmes wrote:
Hi Thomas,
I think I'm going to need a lot more time to understand this issue
completely. The synchronization being used seems complex and somewhat
ill-defined, and also inadequate. I'm not convinced that adding code
inside a synchronized block really fixes a race condition caused by
unsynchronized access - but it may well narrow the window significantly.
It'll be good to get your time to look into it. In fact I suggested
this fix to Thomas while I do raise a question to myself whether we
should attempt to clean up the synchronization in this fix (this code
was implemented before the new Java memory model in 1.5).
Here is my understanding why I think the suggested fix is adequate.
r.queue will be set to ENQUEUED when enqueued and set to NULL when dequeued.
1) Read access to r.queue
a) no lock to read r.queue
in Reference.enqueue() method and ReferenceHandler thread
possibly race is that when calling RQ.enqueue(Reference) method
(i) it's already enqueued
(ii) it's already dequeued
RQ.enqueue(Reference) has to handle that (where the bug is)
b) Reference.isEnqueued holds the Reference object monitor
this.next is null initially and is set by the VM to non-null
pending for enqueue. I believe checking this.next != null
is an optimization.
Potential race is that the reference may be dequeued which
doesn't hold the Reference object lock. Is this a bug?
This race is acceptable in my opinion as I think the isEnqueued
method is useful to determine when a referent has been GC'ed
and then followed by dequeuing it from the queue. If there
are threads that doing dequeuing, the code calling isEnqueued
will prepare for the race when isEnqueued returning true,
the reference may have been dequeued shortly.
Precisely handling the race that isEnqueued returns true while
it may be dequeued doesn't prevent this situation.
2) Write to r.queue
a) acquire Reference object monitor and ReferenceQueue.lock
to set r.queue to ENQUEUED
Potential race in RQ.enqueue method
synchronized (r) {
if (r.queue == ENQUEUED) return false;
synchronized (lock) {
if (r.queue != this) return false;
...
Moving if (r.queue == ENQUEUED) line to after acquiring lock
makes it easier to understand and maintain but I don't see
a race in the existing code without holding lock.
b) acquire ReferenceQueue.lock to set r.queue to NULL
ReferenceQueue.poll and remove methods calling reallyPoll method
I don't know the history. The nested lock inversion problem
is probably the reason why it doesn't acquire the Reference object
monitor to dequeue.
This one is related to whether Reference.isEnqueued() should
return (see above).
Mandy
