Hi Eric,

I've taken a look at the webrev and I mainly agree with the changes/refactoring.

On the issue of increasing the timeout to 20 secs; there is a clear race in this test, and your refactoring has reduced it as much as possible. Since the sleep will always occur, I would prefer to reduce it from 20 secs to 5 secs. That, in combination with your use of a latch, should be sufficient.

Also, there is a variant of each test that runs with a 5,000 millisecond timeout. This timeout should be increased to 20,000 milliseconds. Otherwise, the blocking IO call may timeout rather than be interrupted ( which is what the test is testing for ).

If you address the above issues, I will be happy to sponsor this change for you.

-Chris.

On 28/03/14 05:54, Eric Wang wrote:
Hi Chris & All,

Sorry that the fix spent a bit long time for internal review.
Can you please review the fix below for bug JDK-8025209
<https://bugs.openjdk.java.net/browse/JDK-8025209>. The fix is just to
make the thread synchronization is more robust and made some cleanup.
http://cr.openjdk.java.net/~ewang/JDK-8025209/webrev.00/

Thanks,
Eric
On 2014/3/5 18:01, Chris Hegarty wrote:
On 5 Mar 2014, at 09:48, Eric Wang<yiming.w...@oracle.com>  wrote:

Hi everyone,

Hi Everyone,

I'm working on the test bughttps://bugs.openjdk.java.net/browse/JDK-8025209, 
The test uses Thread.sleep to sync-up threads which is not a correct 
assumption. The fix is just to sync-up 2 threads using proper way.

The webrev will be sent after internal review. Please let me know if you have 
any comment.
This test is part of the OpenJDK source. Please just post your webrev 
externally for review.

Thanks,
-Chris.

Thanks,
Eric


Reply via email to