Looks OK. Some minor comments: - Some of the static fields in the test could be final (queue, weakReference, startedSignal). - After the join() loop in the test you could check that the weakReference is cleared and enqueued. - Why is the check thread.actual < TIMEOUT only done if thread.reference is null? This would seem to be appropriate for both cases. (I like Mandy's suggestion of using || on the conditions). - I agree with Mandy about throwing RuntimeException instead of Exception.
Mike On Feb 25 2014, at 06:22 , Ivan Gerasimov <ivan.gerasi...@oracle.com> wrote: > Thank you Mike! > > On 24.02.2014 22:26, Mike Duigou wrote: >> On Feb 24 2014, at 06:37 , roger riggs <roger.ri...@oracle.com> wrote: >> >>> Hi Ivan, >>> >>> The code is correct as written but there might be some creep in the end >>> time due to the sampling of System.nanoTime. >>> >>> I would be inclined to calculate the final time of the timeout once >>> and then compare simply with the current nanotime. >>> >>> long end = (timeout == 0) ? Long.MAX_VALUE : (System.nanoTime() + timeout * >>> 1000000); >> I hate seeing numerical constants >> >> TimeUnit.MILLISECONDS.toNanos(timeout) > Yes, this is much clearer. > Though I used the opposite conversion: NANOSECONDS.toMillis(end - start); > > Would you please take a look at the updated webrev: > http://cr.openjdk.java.net/~igerasim/6853696/1/webrev/ > > Sincerely yours, > Ivan > >> >>> Then the test in the loop can be: >>> >>> if (System.nanoTime() > end) { >>> return null; >>> } >> This compare should be re-written in the overflow compensating style Martin >> mentions. >> >> Mike >> >>> Roger (Not a Reviewer) >>> >>> On 2/24/2014 12:59 AM, Ivan Gerasimov wrote: >>>> Hello! >>>> >>>> ReferenceQueue.remove(timeout) may return too early, i.e. before the >>>> specified timeout has elapsed. >>>> >>>> Would you please review the fix? >>>> The change also includes a regression test, which can be used to >>>> demonstrate the issue. >>>> >>>> BUGURL: https://bugs.openjdk.java.net/browse/JDK-6853696 >>>> WEBREV: http://cr.openjdk.java.net/~igerasim/6853696/0/webrev/ >>>> >>>> Sincerely yours, >>>> Ivan >> >> >
