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

[David Holmes]

On 30/07/2015 8:41 PM, Peter Levart wrote:
>
>
> 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:

So r has been enqueued and one poll() removes it, so the second poll() 
returns NULL, but r still claims to be enqueued. Sorry I'm not seeing 
how that is possible.

David

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