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
