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

Thomas Schatzl thomas.schatzl at oracle.com
Wed Jun 19 09:51:55 UTC 2013

Hi Peter,

On Wed, 2013-06-19 at 11:05 +0200, Peter Levart wrote:

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

I do not disagree with that, I was just guessing that the synchronization on the reference was for synchronization between isEnqueued() and enqueue(). Not poll() and isEnqueued() for the reasons you mentioned.

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.

Probably.

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.

You are right that volatile would be needed in this case.

While you're at it, the reallyPoll() could optimize the double volatile read from head and only perform it once:

I would be fine with that change, depends on others' opinions. It might be better to make a different CR for cleanups though.

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?

None, as I mentioned, for the original issue. Except that it solves the problem, that if you wanted (as I think has been suggested in the earlier email) to also synchronize on the reference, which is a bad idea because of the different order of the nested synchronization. (I am not sure how also synchronizing on the reference in poll() would improve the situation too)

It would also obviate the need for the "lock" lock in a next step as they are 1<->1. But maybe for some other reasons using an explicit lock object is the preferred way.

Thomas

More information about the core-libs-dev mailing list