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

[David Holmes]

On 25/02/2014 1:49 PM, Ivan Gerasimov wrote:
>
> On 25.02.2014 6:52, David Holmes wrote:
>> On 25/02/2014 1:34 AM, Ivan Gerasimov wrote:
>>> Thank you Roger for looking at the fix!
>>>
>>> On 24.02.2014 18:37, roger riggs 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 don't think it is always correct.
>>> According to the documentation, "The value returned represents
>>> nanoseconds since some fixed but *arbitrary* origin time".
>>> http://download.java.net/jdk8/docs/api/java/lang/System.html#nanoTime--
>>>
>>> Thus, Long.MAX_VALUE may just happen to represent some valid time in the
>>> near future, and we'll achieve wrong result.
>>
>> Correct. Simplest thing is to calculate end ignoring whether timeout
>> is zero, and only check "now < end" if timeout != 0.
>
> David, as Martin said before, 'now < end' may hold true even if now is
> after end (now is negative).
> In my version of the code, I operate on the difference (after - before),
> thus avoiding the possible overflow.

Sorry, I put "now < end" in quotes to mean the effect of now<end, the 
actual calculation should still be a subtraction as Martin indicated. 
However I was overlooking the fact you need to reduce the timeout for 
subsequent waits.

David

> Sincerely yours,
> Ivan
>
>>
>> David
>>
>>> Sincerely yours,
>>> Ivan
>>>
>>>> Then the test in the loop can be:
>>>>
>>>>   if (System.nanoTime() > end) {
>>>>      return null;
>>>>   }
>>>>
>>>> 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
>>>>
>>>>
>>>
>>
>>
>