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
