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.ch...@oracle.com
<mailto:mandy.ch...@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
