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

[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.
http://cr.openjdk.java.net/~mchung/jdk8/webrevs/8022208/webrev.02/

I'll let them run longer to see if it can uncover any race.  How does 
this version look?

Mandy

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.
>
> 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?
>

I'm happy with the new version of ThreadStateController.java with the 
above cleanup.

> 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;

The switch statement is looking at the current state (i.e. RUNNABLE).

The above WAITING and TIMED_WAITING cases indicate that this thread is 
in waiting and thus the request to transition to another state has first 
to notify this thread so that it can proceed with the state transition.


>
>
> 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 this thread is currently in WAITING state, the main thread calls 
goTimedWaiting, before it notifies this thread, this thread gets waken 
up due to spurious wakeup or interrupt, then the race depending on who 
grabs the lock first as you described above will happen.

> 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

That's what the test does.

> - allow myThread to escape from state X

Is this really needed?  This would require the main thread to notify 
myThread the verification is done.  The test currently goes to the new 
state for validation and it seems to me that this isn't needed.

Mandy