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

Peter Levart peter.levart at gmail.com
Wed Jun 19 09:05:23 UTC 2013

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;
             if (r instanceof FinalReference) {
             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

More information about the core-libs-dev mailing list