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

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

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             case RUNNABLE:
  224                 nextState(RUNNABLE);
  225                 break;

The case value, eg RUNNABLE, and the arg to nextState, eg RUNNABLE, are 
two completely different values! Can I suggest using S_xxx for the int 
states (why not an enum?).

Typo: awaitArrive should be awaitAdvance

Thanks,
David

> 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