Code Review 7021010: java/lang/Thread/ fails intermittently

Chris Hegarty chris.hegarty at
Thu Jun 23 10:22:54 UTC 2011

On 06/23/11 06:33 AM, David Holmes wrote:
 > Sorry for the delay on this ...
 > I concur with Mandy that using arrive() the thread must always be
 > RUNNABLE or the expected next state. Hence the new check is ok.

Thanks David,

 > With the new synchronization (and perhaps even the old) it seems to me
 > that here:
 > private void setState(int newState) {
 > switch (state) {
 > case BLOCKED:
 > while (state == BLOCKED) {
 > goSleep(20);
 > }
 > state = newState;
 > break;
 > the sleep loop is unnecessary as after setting the new state the main
 > thread will call arriveAndAwaitAdvance.

In its current form we can't remove this ( I only found out after 
removing it and testing ). The reason is that after the main thread 
releases globalLock, MyThread sets the state to RUNNABLE (to keep it 
busy before the next state is set). But goWaiting may be called on the 
main thread before MyThread sets the state to be RUNNABLE. So the 
WAITING state could be lost and MyThread go into an infinite loop.

I think it may be best to just reduce the sleep from 20ms to 10ms.

 > Also here:
 > try {
 > Thread.sleep(1000000);
 > } catch (InterruptedException e) {
 > // finish sleeping
 > interrupted();
 > }
 > the interrupted() call is redundant as the interrupt state is already
 > cleared when the IE is thrown.

Thanks, I'll remove this redundant call.

Updated Webrev:


 > Cheers,
 > David
 > David Holmes said the following on 06/22/11 20:05:
 >> Mandy,
 >> I need to study the test in more detail to see exactly how the thread
 >> is supposed to change state and in what order. I'm not yet convinced
 >> that arrive() in place of arriveAndAwaitAdvance() doesn't introduce
 >> races.
 >> David
 >> Mandy Chung said the following on 06/22/11 13:18:
 >>> On 6/21/11 7:12 PM, Chris Hegarty wrote:
 >>>> [...]
 >>>> > I'm not sure the extra check in checkThreadState that the thread 
 >>>> > be RUNNABLE is valid. What if you are transitioning the thread 
from a
 >>>> > blocked to non-blocked state, you may still see it blocked on the
 >>>> > first call to getState.
 >>>> > L130-116: I agree with David that this test is not needed.
 >>>> > Like the RuntimeException listed in ProblemList.txt shows
 >>>> > that the target thread is in WAITING state but expected to
 >>>> > be RUNNABLE.
 >>>> The main thread executes goBlocked, goWaiting, goTimedWaiting, etc.
 >>>> These methods set the state and then wait for the phaser to advance.
 >>>> The phaser will not advance until MyThread triggers it, at which
 >>>> point the thread should either be RUNNABLE or the expected state,
 >>>> right? Or have I missed something?
 >>> With the change from calling arriveAndAwaitAdvance to calling arrive,
 >>> I think you're probably right that the thread should either be
 >>> RUNNABLE or the expected state. If calling arriveAndAwaitAdvance, the
 >>> thread delays and waits for the main thread to arrive. The main
 >>> thread may see that the target thread is in a state waiting for
 >>> phaser to advance (depending on the implementation).
 >>>> I added this extra check since we are now relaxing the check for the
 >>>> expected state. I just thought it would be more correct than
 >>>> allowing any state, but if you feel it too strict ( or still
 >>>> incorrect ) I can remove it.
 >>> With the change to call arrive, it seems fine to keep this check. I'd
 >>> like to get David's opinion on this.
 >>>> > I also don't understand why you moved the terminated check to after
 >>>> > the join() - if the thread is failing to terminate then the join(),
 >>>> > which is untimed, will simply not return and the test will hang
 >>>> > until timed-out by the harness.
 >>>> > L98: Are you seeing some timing issue if you keep this
 >>>> > checkThreadState
 >>>> > before the join and thus you moved it after the join?
 >>>> No timing issue. I did this for simplicity, given that the state
 >>>> should be TERMINATED when join returns. Either way must
 >>>> complete before the threads state will be set to TERMINATED.
 >>>> Invoking checkThreadState before that point just seems more likely
 >>>> encounter retries. I'm ok with either, just let me know if you want
 >>>> it reverted back to the original.
 >>> The thread could move to TERMINATED state once it completes execution
 >>> and while the main thread is not yet notified. Since there is no
 >>> timing issue and MyThread is calling arrive rather than
 >>> arriveAndAwaitsAdvance, I would say keep the checkThreadState call
 >>> before the join.
 >>>> > I also don't think the use of the Phaser is appropriate here as you
 >>>> > are actually delaying the thread from making the state change. 
In the
 >>>> > original code the target thread signals the main thread that it is
 >>>> > about to go to state X and continues to advance to state X (modulo
 >>>> > preemption etc). But with the Phaser the target thread indicates it
 >>>> > is about to go to state X and then waits for the main thread -
 >>>> > consequently it is more likely that when the main thread calls
 >>>> > checkThreadState that the target has not yet reached the desired
 >>>> > state and so the main thread will have
 >>>> > to loop. This isn't incorrect it just seems to me that in the 
 >>>> > configuration the test may not take a lot longer in relative terms.
 >>>> > Maybe the additional clarity is worth it though ...
 >>>> When MyThread invokes arriveAndAwaitAdvance, then it should be the
 >>>> final party to arrive and therefore "probably" shouldn't wait. But
 >>>> you raise a good point, myThread should really just invoke
 >>>> phaser.arrive() rather that arriveAndAwaitAdvance.
 >>>> Updated Webrev:
 >>> Looks good. Thanks for fixing this timing issue.
 >>> Mandy

More information about the core-libs-dev mailing list