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

[Ivan Gerasimov]

Thanks David!

On 26.02.2014 8:44, David Holmes wrote:
> Hi Ivan,
>
>  143             long start = (timeout == 0) ? 0 : System.nanoTime();
>
> This can simply be:
>
>  143             long start = System.nanoTime();
>
> If timeout==0 then you will never execute the code that even looks at 
> start.
>
That's right.
System.nanoTime() isn't too expensive, but it's still a native function 
which may end up with a syscall, so I tried to avoid calling it unless 
necessary.

Sincerely yours,
Ivan

> Cheers,
> David
>
> On 26/02/2014 7:46 AM, Ivan Gerasimov wrote:
>> Thank you Mandy!
>>
>> On 25.02.2014 21:53, Mandy Chung wrote:
>>> On 2/25/14 6:32 AM, Ivan Gerasimov wrote:
>>>>> line 76: why do you want to catch InterruptedException?  If
>>>>> interrupted, should the test fail?
>>>> ReferenceQueue#remove() can throw InterruptedException, so I had to
>>>> handle it.
>>>> In the new webrev I set the initial value of actual to TIMEOUT, so if
>>>> the thread is interrupted the test will pass.
>>>>
>>>
>>> Catching the InterruptedException would hide any unexpected issue as
>>> the test doesn't really expect to be interrupted. For example if the
>>> test is interrupted (kill -9), we should let the test to fail which is
>>> fine.  I suggest not to catch it.
>>>
>> Ok. I changed the code to throw RuntimeException.
>>
>>> line 61:  I think the test should be:
>>>    if (thread.reference != null || thread.actual < TIMEOUT)
>>>
>> Sorry, I'm not clear why.
>> 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().
>>
>> Please find the slightly updated version of webrev here:
>> http://cr.openjdk.java.net/~igerasim/6853696/2/webrev/
>> I didn't change the if () clause, as I'm not yet sure why it should be
>> done.
>>
>> Sincerely yours,
>> Ivan
>>
>>> you may want to update the exception. You could simply throw
>>> RuntimeException (just a minor point).
>>>
>>>> Please take a look at the updated webrev:
>>>> http://cr.openjdk.java.net/~igerasim/6853696/1/webrev/
>>>
>>> Looks okay.  I have a slight preference to keep the simple division to
>>> convert to millis rather than loading the enum TimeUnit class for this
>>> purpose.
>>>
>>> Mandy
>>>
>>>
>>
>