RFR (XS): 8014890 : Reference queues may return more entries than expected (original) (raw)
Thomas Schatzl thomas.schatzl at oracle.com
Wed Jun 19 14:09:51 UTC 2013
- Previous message: RFR (XS): 8014890 : Reference queues may return more entries than expected
- Next message: RFR (XS): 8014890 : Reference queues may return more entries than expected
- Messages sorted by: [ date ] [ thread ] [ subject ] [ author ]
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
- Previous message: RFR (XS): 8014890 : Reference queues may return more entries than expected
- Next message: RFR (XS): 8014890 : Reference queues may return more entries than expected
- Messages sorted by: [ date ] [ thread ] [ subject ] [ author ]