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

[Mike Duigou]

Looks OK. Some minor comments:

- Some of the static fields in the test could be final (queue, weakReference, startedSignal).
- After the join() loop in the test you could check that the weakReference is cleared and enqueued. 
- 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).
- I agree with Mandy about throwing RuntimeException instead of Exception.

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