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. 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). > > 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
