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

[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 don't see how this works - and that applies to the old 
> version too! (Which I know I've looked at before so this is somewhat 
> unsettling for me :( ).
>
> First I found it confusing to track which thread was which in the 
> refactoring (which in itself is a good idea). Inside 
> ThreadStateController "myThread" doesn't mean anything - that comes 
> from the calling context - I suggest changing to refer to "this 
> thread" or "the current thread" as appropriate. eg "Have the current 
> thread wait until this thread is about to block" or whatever is needed.
>

I wasn't happy with "myThread" too and should rename it.

> Looking at an actual test we have eg:
>
>    myThread.goWaiting();
>   ThreadStateController.checkThreadState(myThread, Thread.State.WAITING);
>
> First: why is checkThreadState static rather than just an instance 
> method?
>

Yes it can.

> So goWaiting is supposed to tell myThread to go to a "waiting" state 
> so that the main thread can then verify that. Lets assume for 
> arguments sake that the thread is currently RUNNABLE so it is 
> currently looping around in run() doing the little math exercise. 
> goWaiting does:
>
>   public void goWaiting() {
>     System.out.println("Waiting myThread to go waiting.");
>     setState(WAITING);
>     // wait for MyThread to get to just before wait on object.
>     phaser.arriveAndAwaitAdvance();
>  }
>
> and setState does:
>
>    case WAITING:
>    case TIMED_WAITING:
>        state = newState;
>        synchronized (lock) {
>           lock.notify();
>        }
>        break;
>
>
> so as soon as we update "state" myThread is capable of changing what 
> it is doing in run() to:
>
>    case WAITING: {
>       synchronized (lock) {
>          // signal main thread.
>          phaser.arrive();
>          System.out.println("  myThread is going to wait.");
>          try {
>              lock.wait();
>           } catch (InterruptedException e) {
>              // ignore
>              interrupted.incrementAndGet();
>           }
>       }
>       break;
>
> so now we have a race between the two threads to see who can grab the 
> lock first. If it is the main thread then we issue a notify when 
> nothing is waiting and so the subsequent wait() by myThread will 
> potentially wait forever. At least in that case the main thread will 
> see that it is waiting!
>
> If "myThread" wins the race it will wait after signalling the phaser 
> and the main thread will then issue the notify allowing myThread to 
> proceed (and do what? process the WAITING case again??). So there is 
> no guarantee that myThread will be waiting when the main thread checks 
> the state!
>
> Similarly for issuing the unpark in the parking cases.
>
> AFAICS the basic approach here should be:
> - tell myThread to go to state X
> - wait until myThread should be in state X
> - verify myThread is in state X
> - allow myThread to escape from state X
>
> but this code does the last step second.
>
> ???
>

I was looking at the intermittent failures and fix the problem I 
identified.   What you said above is definitely potential race that I 
didn't have analyzed the waiting state thoroughly.  It's time to revisit 
the whole test and see if there is a better way to code this.

thanks for the review.
Mandy