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

[David Holmes]

Hi Thomas,

On 19/06/2013 7:08 AM, Thomas Schatzl wrote:
> Hi all,
>
>    can I have reviews for the following change?
>
> It happens if multiple threads are enqueuing and dequeuing reference
> objects into a reference queue, that Reference objects may be enqueued
> at multiple times.
>
> This is because when java.lang.ref.ReferenceQueue.poll() returns and
> inactivates a Reference object, another thread may just be during
> enqueuing it again.

Are we talking about different queues here? Otherwise both poll() and 
enqueue() are using synchronized(lock). I also note that enqueue 
synchronizes on the Reference itself, which suggests that poll (actually 
reallyPoll) should also be synchronizing on the reference (though we 
have a nested lock inversion problem that would need to be solved).

David
-----

> In the test case provided, the two threads that conflict are the
> reference handler thread and the program (main) thread.
>
> Relevant code:
>
> ReferenceHandlerThread.run():
>
> 0: [...]
> 1: ReferenceQueue q = r.queue; // r is the reference
> 2: if (r != ReferenceQueue.NULL)
> 3:   q.enqueue().
>
> ReferenceQueue::poll()(reallyPoll()) code (I removed lots of code here)
>
> 1: synchronized(lock) {
> 2:   [...]
> 3:   r.queue = ReferenceQueue.NULL;
> 3:}
>
> I.e. while ReferenceQueue.poll() sets the Reference's queue to the NULL
> queue so that that reference will not be enqueued again (or at most into
> the NULL queue which is a nop), it happens that if the task switch
> occurs between lines 2 and 3 of the reference handler thread, q still
> contains the old queue reference, and the reference handler thread will
> enqueue the Reference into the original queue again.
>
> You can achieve the same effect by simply calling
> ReferenceQueue.enqueue() (i.e. without the reference handler thread, or
> within the reference handler thread doing the != NULL check), it's just
> that in such a case the "old" ReferenceQueue is stored in some register.
>
> The guard for ReferenceQueue.NULL does not have any effect except for
> possibly saving the virtual call. Simply calling r.enqueue() exhibits
> the same problem.
>
> The proposed solution is to filter out References within
> ReferenceQueue.enqueue() again. At that point we can check whether the
> given Reference is actually meant for this queue or not. Already removed
> References are known to be "inactive" (as enqueue and poll are mutually
> exclusive using a lock), in particular the References' queue is
> different (actually the NULL queue) to the queue it is being enqueued.
>
> This change should pose no change in semantics, as the ReferenceQueue of
> the Reference can only be set in its constructor, and as soon as the
> Reference is removed from a ReferenceQueue, its ReferenceQueue will be
> the NULL queue. (I.e. before this change you could not enqueue such an
> "inactive" Reference multiple times anyway)
>
> (too many References and queues here :)
>
> Webrev with test case
> http://cr.openjdk.java.net/~tschatzl/8014890/webrev/
>
> JIRA:
> https://jbs.oracle.com/bugs/browse/JDK-8014890
>
> bugs.sun.com
> http://bugs.sun.com/view_bug.do?bug_id=8014890
>
> Testing:
> jck, jprt, manual testing
>
> Note that I also need a sponsor to push in case this change is approved.
>
> Thanks,
>    Thomas
>