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

Ivan Gerasimov ivan.gerasimov at oracle.com
Tue Feb 25 21:46:02 UTC 2014


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




More information about the core-libs-dev mailing list