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

[David Holmes]

Chris Hegarty said the following on 06/23/11 20:22:
> 
> 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.

Ouch! That's a nasty race. :(

David
-----

> 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:
>   http://cr.openjdk.java.net/~chegar/7021010/jdk8.webrev.02/webrev/
> 
> -Chris
> 
>  >
>  > 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 
> must
>  >>>> > 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 MyThread.run 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 
> "wrong"
>  >>>> > 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:
>  >>>> http://cr.openjdk.java.net/~chegar/7021010/jdk8.webrev.01/webrev/
>  >>> Looks good. Thanks for fixing this timing issue.
>  >>> Mandy