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

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

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.

???

David
-----

> Mandy
>
> On 10/31/2013 11:22 AM, Mandy Chung wrote:
>>
>> 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 it and add the shared
>> code in the testlibrary.    Will send out a revised webrev.
>>
>> Mandy
>>
>>>
>>> On Thu, Oct 31, 2013 at 10:53 AM, Mandy Chung <mandy.chung at oracle.com
>>> <mailto:mandy.chung at oracle.com>> wrote:
>>>
>>>     https://bugs.openjdk.java.net/browse/JDK-8022208
>>>
>>>     Webrev at:
>>>
>>> http://cr.openjdk.java.net/~mchung/jdk8/webrevs/8022208/webrev.00/
>>> <http://cr.openjdk.java.net/%7Emchung/jdk8/webrevs/8022208/webrev.00/>
>>>
>>>     The retry loop in checking the thread state assumes that the
>>>     thread state is in RUNNABLE state which isn't always the case (it
>>>     could be any other state).  The fix is to remove that check and
>>>     the thread should be a daemon thread so that the test can
>>>     terminate if any exception is thrown.
>>>
>>>     jdk/test/java/lang/management/ThreadMXBean/ThreadStateTest.java
>>>     is a similar test that performs additional validation on the
>>>     ThreadMXBean API.  It should also be fixed as a daemon thread I
>>>     take the opportunity to change it to use
>>>     java.util.concurrent.Phaser instead of the utility class.
>>>
>>>     Mandy
>>>
>>>
>>
>