Hi Thomas,
I think I'm going to need a lot more time to understand this issue
completely. The synchronization being used seems complex and somewhat
ill-defined, and also inadequate. I'm not convinced that adding code
inside a synchronized block really fixes a race condition caused by
unsynchronized access - but it may well narrow the window significantly.
David
On 19/06/2013 5:34 PM, Thomas Schatzl wrote:
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
