RFR: 8132306: java/lang/ref/ReferenceEnqueue.java fails with "RuntimeException: Error: poll() returned null; expected ref object"

[Peter Levart]

On 07/30/2015 12:24 PM, David Holmes wrote:
> On 30/07/2015 8:20 PM, Peter Levart wrote:
>>
>>
>> On 07/30/2015 11:12 AM, Daniel Fuchs wrote:
>>> Hi Peter,
>>>
>>> I'm glad you're looking at this too! We can't have too many eyes :-)
>>>
>>> On 30/07/15 10:38, Peter Levart wrote:
>>>> Suppose we have a Reference 'r' and it's associated ReferenceQueue 
>>>> 'q'.
>>>> Currently it can happen that the following evaluates to true, which is
>>>> surprising:
>>>>
>>>> q.poll() == null && r.isEnqueued()
>>>>
>>>
>>> But on the other hand this can only happen if two different
>>> threads are polling the queue - in which case only one of them
>>> will get the reference. In such a situation, the symmetric condition
>>> would not be surprising (as the other thread would
>>> get q.poll() != null):
>>>
>>>    r.isEnqueued() && q.poll() == null
>>>
>>> The original bug fixed by Kim is more surprising, because there's only
>>> one Thread doing the polling, and it does get:
>>>
>>>    r.isEnqueud() && q.poll() == null
>>>
>>> although nobody else is polling the queue.
>>> This should no longer occur in this situation with Kim's fix.
>>>
>>> cheers,
>>>
>>> -- daniel
>>
>> Yes, these are two different issues. The one Kim has fixed is the race
>> of above expressions with Queue.enqueue() (when the queue is changing
>> from the associated instance to ENQUEUED). The one I'm pointing at and
>> Kim has already identified as potential issue is the race of the
>> following expression:
>>
>> r.isEnqueued() && q.poll() == null && r.isEnqueued() ==> true
>>
>> ....with Queue.[realy]poll() (when the queue is changing from ENQUEUED
>> to NULL).
>>
>> Which only happens if at least two threads are polling the queue, but is
>> equally surprising and prone to cause bugs, don't you think?
>
> Sorry I'm missing your point. The expression:
>
> r.isEnqueued() && q.poll() == null
>
> is exactly the race the current fix is addressing. Adding a second 
> check of r.isEnqueued() which still returns true does not add anything 
> that I can see. ??
>
> David

The expressions are similar, but the race is different. The one 
addressed by Kim is the race of:

r.isEnqueued() && q.poll() == null ==> true

with q.enqueue(r)

There, the reversal of assignments to q.head and r.queue in 
ReferenceQueue.enqueue() fixes the issue.


The one I'm pointing at is the race of:

r.isEnqueued() && q.poll() == null && r.isEnqueued() ==> true

with q.poll()

Here, the fix would be to also revert the assignments to q.head and 
r.queue in ReferenceQueue.reallyPoll() this time.


The 1st race is the race with enqueue-ing and the 2nd race is the race 
with de-queueing. Initially, my "surprising" expression was:

q.poll() == null && r.isEnqueued() ==> true


...but this is not representative, as it is also an expected outcome 
when racing with enqueue-ing. So I added a pre-condition to express the 
fact that it happens when racing with de-queueing only:

r.isEnqueued() && q.poll() == null && r.isEnqueued() ==> true


What is surprising in the above expression evaluating to true is the 
fact that 'r' appears to be enqueued before and after the q.poll() 
returns null. I can easily imagine code that would fail because it never 
imagined above expression to evaluate to true. For example:

if (r.isEnqueued()) {
     Reference s = q.poll();
     if (s == null) {
         // somebody else already dequeued 'r'
         assert !r.isEnqueued();
     }
}



Regards, Peter

>
>> Regards, Peter
>>
>>