RFR  (ref) ReferenceQueue.remove(timeout) may return null even if timeout has not expired
ivan.gerasimov at oracle.com
Wed Feb 26 05:47:59 UTC 2014
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
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
> 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:
>> I didn't change the if () clause, as I'm not yet sure why it should be
>> Sincerely yours,
>>> you may want to update the exception. You could simply throw
>>> RuntimeException (just a minor point).
>>>> Please take a look at the updated 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
More information about the core-libs-dev