David,
Here is the new version, I have added the comments as you suggested, and
I replaced the variable "sources" with a synchronized list.
http://cr.openjdk.java.net/~sjiang/JDK-8068418/01/
Thanks for the review.
Shanliang
David Holmes wrote:
Hi Shanliang,
On 6/01/2015 3:47 AM, shanliang wrote:
Hi,
Please review this fix:
bug: https://bugs.openjdk.java.net/browse/JDK-8068418
webrev: http://cr.openjdk.java.net/~sjiang/JDK-8068418/00/
This must be a timing issue in the test, the test called:
t.join(5000L); to wait a thread's dying, I reproduced the bug by
insert at line 202:
try {
Thread.sleep(5100);
} catch (Exception ee) {}
to delay the t's dying.
The fix is to replace:
t.join(5000L);
by:
t.join();
and replace:
Object.wait(timeout);
by
CountDownLatch.countDown();
Okay - you could have just done wait() but the switch to
CountDownLatch is somewhat cleaner.
The test harness timeout will be used as the max waiting timeout.
So the test will now never report that it thinks it has hit a
deadlock, it will just timeout. But you added some extra printout so
okay I guess - but I suggest adding a comment at the top (where @run
etc are) saying that if test times out then deadlock is suspected.
Minor nit:
System.out.println("DeadlockTest-addNotificationListener waiting the
XXX thread to die.
should say "waiting for the XXX thread ..."
Thanks,
David
Shanliang
