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

[David Holmes]

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