Hi, one more note :)
On Wed, 2013-06-19 at 09:28 +0200, Thomas Schatzl wrote: > Hi again, > > some correction... > > On Wed, 2013-06-19 at 09:17 +0200, 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. > > This also guards against multiple concurrent calls to enqueue on the > same reference - so this statement is needed after all. Actually, with that patch (e.g. the if (this != r.queue check) return false; ) this situation would also be covered I think. But that's unrelated, sorry. Thomas
