On 06/19/2013 09:17 AM, Thomas Schatzl wrote:
Hi,
On Wed, 2013-06-19 at 12:47 +1000, David Holmes wrote:
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).
This does not help imo. Still there may be temporary storage containing the
original ReferenceQueue reference, and .enqueue() gets called on it with the
now inactive Reference.
Enqueue() and (really-)poll() themselves already synchronize on the
"lock" lock to guard modification of the queue, this is safe.
The synchronize(r) statement in ReferenceQueue.enqueue() is about
synchronization with Reference.isEnqueued() I think. Actually I am not sure
whether the synchronization between isEnqueued() and enqueue() makes a
difference.
Hi Thomas,
It doesn't make a difference between Reference.isEnqueued() and
ReferenceQueue.poll(), since there isn't any synchronization between the
two. So isEnqueued() can still be returning true while another thread
has already de-queued the instance. I guess the real use of isEnqueued()
method is reliable detection of false -> true transition only.
I can't see why the isEnqueued() method checks for both (next != null &&
queue == ENQUEUED). Wouldn't the check for (queue == ENQUEUED) be
enough? In that case the Reference.queue field could be made volatile
and synchronization on Reference instance removed.
While you're at it, the reallyPoll() could optimize the double volatile
read from head and only perform it once:
private Reference<? extends T> reallyPoll() { /* Must hold
lock */
Reference<? extends T> r = head;
if (r != null) {
head = (r.next == r) ? null : r.next;
r.queue = NULL;
r.next = r;
queueLength--;
if (r instanceof FinalReference) {
sun.misc.VM.addFinalRefCount(-1);
}
return r;
}
return null;
}
Another solution would be to synchronize enqueue() and poll() on the queue
itself, and check whether the reference passed to enqueue() is inactive or not
(i.e. this check is still needed).
ReferenceQueue.this and ReferenceQueue.lock are 1<->1. What difference
would that make?
Regards, Peter
ReferenceHandlerThread.run():
0: [...]
1: ReferenceQueue q = r.queue; // r is the reference
2: if (r != ReferenceQueue.NULL)
3: q.enqueue().
Between lines 2 and 3, q contains a reference to the old ReferenceQueue (which
is not ReferenceQueue.NULL). So if the thread is switched there, i.e. the main
thread does a poll() on q, the main thread makes r inactive. When switching
back to the reference handler thread (or any other thread), enqueue() of that
Reference r will still be called on the original q. The actual r.queue is
already ReferenceQueue.NULL from the poll(). (i.e. the Reference is inactive).
This change guards against that situation as this is (imo) an unexpected
behavior (that you can enqueue a Reference multiple times; and an already
inactive one too).
The only solution would be synchronizing on r in the ReferenceHandler code to
avoid that (I think). However then everyone that uses Reference.enqueue()
(which uses ReferenceQueue.enqueue()) would need to synchronize on the
Reference to make the code thread safe. I haven't seen that in the
documentation.
As mentioned this situation may occur independently of whether the
ReferenceHandler thread is involved or not.
I.e. if you look at Reference.enqueue() which reads as
this.queue.enqueue(this)
This is the same situation, if you consider that "this.queue" may be
temporarily stored in e.g. a register during the thread switch.
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
