RFR (XS): 8014890 : Reference queues may return more entries than expected

[Mandy Chung]

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