Re: Review request for 8022208: Intermittent test failures in java/lang/Thread/ThreadStateTest.java

2013-11-05 Thread Chris Hegarty
On 05/11/2013 03:20, Mandy Chung wrote: David, Rereading your comment and I think you misread the switch statement (see my comments below). In any case, I revisited ThreadStateController.java and looked int the potential races and have done further cleanup. Here is the updated webrev.

Re: Review request for 8022208: Intermittent test failures in java/lang/Thread/ThreadStateTest.java

2013-11-05 Thread Mandy Chung
On 11/4/2013 8:42 PM, David Holmes wrote: This looks good to me. One nit that caused me some confusion - it took me a while to realize that in transitionTo eg: 221 public void transitionTo(Thread.State tstate) throws InterruptedException { 222 switch (tstate) { 223

Re: Review request for 8022208: Intermittent test failures in java/lang/Thread/ThreadStateTest.java

2013-11-05 Thread David Holmes
Thanks Mandy. One minor nit: Having goSleep be an instance method on ThreadStateController makes the usage look weird: thread.goSleep(10); // actually makes current thread go sleep I suggest making it static and use static import to shorten the call site :) If you do change that, no need

Re: Review request for 8022208: Intermittent test failures in java/lang/Thread/ThreadStateTest.java

2013-11-05 Thread Mandy Chung
On 11/5/2013 5:07 PM, David Holmes wrote: Thanks Mandy. One minor nit: Having goSleep be an instance method on ThreadStateController makes the usage look weird: thread.goSleep(10); // actually makes current thread go sleep Good catch. I changed it to a static pause(long ms) method.

Re: Review request for 8022208: Intermittent test failures in java/lang/Thread/ThreadStateTest.java

2013-11-04 Thread Mandy Chung
David, Rereading your comment and I think you misread the switch statement (see my comments below). In any case, I revisited ThreadStateController.java and looked int the potential races and have done further cleanup. Here is the updated webrev.

Re: Review request for 8022208: Intermittent test failures in java/lang/Thread/ThreadStateTest.java

2013-11-04 Thread David Holmes
Hi Mandy, On 5/11/2013 1:20 PM, Mandy Chung wrote: David, Rereading your comment and I think you misread the switch statement (see Yes I did - many times - sorry. my comments below). In any case, I revisited ThreadStateController.java and looked int the potential races and have done

Re: Review request for 8022208: Intermittent test failures in java/lang/Thread/ThreadStateTest.java

2013-10-31 Thread Mandy Chung
On 10/31/2013 11:01 AM, Martin Buchholz wrote: +iterations++; Using ++ on a volatile int looks racy. Using an AtomicInteger is strictly more reliable. Oh that's right. Will fix that. I don't really like duplicating the code in these 2 tests and I am going to refactor

Re: Review request for 8022208: Intermittent test failures in java/lang/Thread/ThreadStateTest.java

2013-10-31 Thread Mandy Chung
Updated webrev that has a new test/lib/testlibrary/ThreadStateController.java and also change to use AtomicInteger: http://cr.openjdk.java.net/~mchung/jdk8/webrevs/8022208/webrev.01/ Mandy On 10/31/2013 11:22 AM, Mandy Chung wrote: On 10/31/2013 11:01 AM, Martin Buchholz wrote: +

Re: Review request for 8022208: Intermittent test failures in java/lang/Thread/ThreadStateTest.java

2013-10-31 Thread Alan Bateman
On 31/10/2013 19:11, Mandy Chung wrote: Updated webrev that has a new test/lib/testlibrary/ThreadStateController.java and also change to use AtomicInteger: http://cr.openjdk.java.net/~mchung/jdk8/webrevs/8022208/webrev.01/ Mandy Is ThreadStateController general enough for the unname package

Re: Review request for 8022208: Intermittent test failures in java/lang/Thread/ThreadStateTest.java

2013-10-31 Thread Mandy Chung
On 10/31/2013 12:34 PM, Alan Bateman wrote: On 31/10/2013 19:11, Mandy Chung wrote: Updated webrev that has a new test/lib/testlibrary/ThreadStateController.java and also change to use AtomicInteger: http://cr.openjdk.java.net/~mchung/jdk8/webrevs/8022208/webrev.01/ Mandy Is

Re: Review request for 8022208: Intermittent test failures in java/lang/Thread/ThreadStateTest.java

2013-10-31 Thread David Holmes
Hi Mandy, On 1/11/2013 5:11 AM, Mandy Chung wrote: Updated webrev that has a new test/lib/testlibrary/ThreadStateController.java and also change to use AtomicInteger: http://cr.openjdk.java.net/~mchung/jdk8/webrevs/8022208/webrev.01/ Sorry but I don't see how this works - and that applies to

Re: Review request for 8022208: Intermittent test failures in java/lang/Thread/ThreadStateTest.java

2013-10-31 Thread Mandy Chung
On 10/31/2013 5:38 PM, David Holmes wrote: Hi Mandy, On 1/11/2013 5:11 AM, Mandy Chung wrote: Updated webrev that has a new test/lib/testlibrary/ThreadStateController.java and also change to use AtomicInteger: http://cr.openjdk.java.net/~mchung/jdk8/webrevs/8022208/webrev.01/ Sorry but I