On 05/11/2013 03:20, Mandy Chung wrote:
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/
The latest webrev looks good to me.
As the last person to touch java/lang/Thread/ThreadStateTest.java,
retrofitting it with Phaser. I can only apologize for not updating
java/lang/management/ThreadMXBean/ThreadStateTest.java then. Thanks for
doing this now, as part of this bug.
-Chris.
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
