Thanks you Mike!
On 25.02.2014 22:47, Mike Duigou wrote:
Looks OK. Some minor comments:
- Some of the static fields in the test could be final (queue, weakReference,
startedSignal).
Done.
- After the join() loop in the test you could check that the weakReference is
cleared and enqueued.
I added the check the weakReference is cleared, but it's not enqueued.
When the reference is removed from the queue, its queue member is set to
NULL.
And checking that the reference is enqueued is actually making sure that
(this.queue == ReferenceQueue.ENQUEUED), which doesn't hold.
- 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).
Sorry, I don't understand why.
As I wrote in the reply to Mandy:
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().
- I agree with Mandy about throwing RuntimeException instead of Exception.
Done.
BTW.
My IDE suggests that a private field ReferenceQueue.queueLength is
initialized, but never used.
Wouldn't it be appropriate to remove it with this change?
Sincerely yours,
Ivan
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
