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

[David Holmes]

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.

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