RFR [6853696] (ref) ReferenceQueue.remove(timeout) may return null even if timeout has not expired

[Ivan Gerasimov]

Thanks you Mike!

On 25.02.2014 22:47, Mike Duigou wrote:
> Looks OK. Some minor comments:
>
> - Some of the static fields in the test could be final (queue, weakReference, startedSignal).
Done.
> - After the join() loop in the test you could check that the weakReference is cleared and enqueued.
I added the check the weakReference is cleared, but it's not enqueued.
When the reference is removed from the queue, its queue member is set to 
NULL.
And checking that the reference is enqueued is actually making sure that 
(this.queue == ReferenceQueue.ENQUEUED), which doesn't hold.

> - Why is the check thread.actual < TIMEOUT only done if thread.reference is null? This would seem to be appropriate for both cases. (I like Mandy's suggestion of using || on the conditions).
Sorry, I don't understand why.
As I wrote in the reply to Mandy:

We have two threads:
1) The lucky one gets non-null reference when it calls remove(). For 
this thread the actual time it had spent on waiting may be much less 
than 1 sec timeout.
2) The one which receives null from remove(). The amount of time it 
should have waited before returning from remove() should not be less 
than timeout.

That's what we should check here:
if the thread is not the lucky one (reference == null), we make sure it 
spent whole second waiting in remove().

> - I agree with Mandy about throwing RuntimeException instead of Exception.
Done.

BTW.
My IDE suggests that a private field ReferenceQueue.queueLength is 
initialized, but never used.
Wouldn't it be appropriate to remove it with this change?

Sincerely yours,
Ivan

> Mike
>
> On Feb 25 2014, at 06:22 , Ivan Gerasimov <ivan.gerasimov at oracle.com> wrote:
>
>> Thank you Mike!
>>
>> On 24.02.2014 22:26, Mike Duigou wrote:
>>> On Feb 24 2014, at 06:37 , roger riggs <roger.riggs at oracle.com> wrote:
>>>
>>>> Hi Ivan,
>>>>
>>>> The code is correct as written but there might be some creep in the end time due to the sampling of System.nanoTime.
>>>>
>>>> I would be inclined to calculate the final time of the timeout once
>>>> and then compare simply with the current nanotime.
>>>>
>>>> long end = (timeout == 0) ? Long.MAX_VALUE : (System.nanoTime() + timeout * 1000000);
>>> I hate seeing numerical constants
>>>
>>> TimeUnit.MILLISECONDS.toNanos(timeout)
>> Yes, this is much clearer.
>> Though I used the opposite conversion: NANOSECONDS.toMillis(end - start);
>>
>> Would you please take a look at the updated webrev:
>> http://cr.openjdk.java.net/~igerasim/6853696/1/webrev/
>>
>> Sincerely yours,
>> Ivan
>>
>>>> Then the test in the loop can be:
>>>>
>>>>   if (System.nanoTime() > end) {
>>>>      return null;
>>>>   }
>>> This compare should be re-written in the overflow compensating style Martin mentions.
>>>
>>> Mike
>>>
>>>> Roger (Not a Reviewer)
>>>>
>>>> On 2/24/2014 12:59 AM, Ivan Gerasimov wrote:
>>>>> Hello!
>>>>>
>>>>> ReferenceQueue.remove(timeout) may return too early, i.e. before the specified timeout has elapsed.
>>>>>
>>>>> Would you please review the fix?
>>>>> The change also includes a regression test, which can be used to demonstrate the issue.
>>>>>
>>>>> BUGURL: https://bugs.openjdk.java.net/browse/JDK-6853696
>>>>> WEBREV: http://cr.openjdk.java.net/~igerasim/6853696/0/webrev/
>>>>>
>>>>> Sincerely yours,
>>>>> Ivan
>>>
>
>