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

[Thomas Schatzl]

Hi,

On Wed, 2013-06-19 at 17:56 +0400, Aleksey Shipilev wrote:
> On 06/19/2013 04:05 PM, David Holmes wrote:
> > 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.
> 
> +1. Having spent 30 minutes trying to figure out the synchronization
> protocol for R and RQ, I can say that is a haunted part of JDK.
> 

:) I had the same problem when trying to figure out the code.

> That said, I think the patch fixes the concrete issue. We are
> piggybacking on the mutual exclusion on RQ.lock to protect ourselves
> from the concurrent mutation of r.queue. The only two places where we
> mutate it is RQ.enqueue() and RQ.poll(), both secured with RQ.lock.
> Given the r.queue is not volatile, we should also acquire the same lock
> while reading r.queue, otherwise races obliterate the reasoning about
> the correctness.
>
> With that notion in mind, I start to wonder if we just need to move the
> check in enqueue() down into the synchronized(lock), and extend it to
> capture NULL:
> 
> --- a/src/share/classes/java/lang/ref/ReferenceQueue.java	Mon Jun 17
> 16:28:22 2013 +0400
> +++ b/src/share/classes/java/lang/ref/ReferenceQueue.java	Wed Jun 19
> 17:53:24 2013 +0400
> @@ -56,8 +56,9 @@
> 
>      boolean enqueue(Reference<? extends T> r) { /* Called only by
> Reference class */
>          synchronized (r) {
> -            if (r.queue == ENQUEUED) return false;
>              synchronized (lock) {
> +                if (r.queue == ENQUEUED || r.queue == NULL)
> +                    return false;
>                  r.queue = ENQUEUED;
>                  r.next = (head == null) ? r : head;
>                  head = r;
> 
> Will it be equivalent to what Thomas is trying to accomplish?

Yes, this is equivalent. Instead of checking the separate ENQUEUED and
NULL values I simply used an "r.queue != this" check which would get
both cases. R.queue cannot have other values than ENQUEUED, NULL or the
queue's value set at initialization.

I wanted to minimize the changes for this particular CR.

Thanks,
Thomas