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
