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