Race condition in ThreadGroup stop test

[Chris Hegarty]

On 11/ 7/11 10:45 PM, David Holmes wrote:
> On 8/11/2011 6:50 AM, Brian Goetz wrote:
>> Wait/notify may be better than sleeping, but semaphore/latch/barrier
>> are much better than wait/notify. Much.
>
> Don't be hasty. This test is using Thread.stop - which as we all know is
> a Very Bad Thing. You want to be very sure that anything that may be
> executing when the async exception hits is async-exception safe. That
> generally excludes all the j.u.c utilities.

Ah yes, I should have known this. Thanks for catching David. We really 
need to be careful with these old tests or we could end up losing their 
original meaning.

Gary, please ignore my previous comments.

-Chris.

>
> Gary's suggested fix is on the right track but needs a few corrections:
>
> On 8/11/2011 2:03 AM, Gary Adams wrote:
>  > diff --git a/test/java/lang/ThreadGroup/Stop.java
>  > b/test/java/lang/ThreadGroup/Stop.java
>  > --- a/test/java/lang/ThreadGroup/Stop.java
>  > +++ b/test/java/lang/ThreadGroup/Stop.java
>  > @@ -32,6 +32,7 @@
>  > private static Thread first=null;
>  > private static Thread second=null;
>
> second needs to be volatile.
>
> Also the Stop() constructor needs to set second after the thread is
> started, otherwise the check below may see second as non-null before it
> has actually started.
>
>  > private static ThreadGroup group = new ThreadGroup("");
>  > + private static boolean groupStopped =false ;
>
> Also needs to be volatile.
>
>  > Stop() {
>  > Thread thread = new Thread(group, this);
>  > @@ -47,8 +48,11 @@
>  > while (true) {
>  > try {
>  > Thread.sleep(1000); // Give other thread a chance to start
>  > - if (Thread.currentThread() == first)
>  > + if ((Thread.currentThread() == first)
>  > + && (second != null)) {
>  > + groupStopped = true;
>  > group.stop();
>
> You need to set groupStopped after group.stop() else the main thread may
> see it before the actual stop happens. As group.stop will throw an
> exception it will need to be set in a finally block.
>
> Now we could remove the sleeps and replace with wait/notify, as they are
> also async-safe, but there is no need. Because the sleeps are being used
> in a polling loop they don't have any issue with slow machines or
> over-loaded machines etc. The sleep logic is simpler than the
> corresponding wait/notify.
>
> David
> -----
>
>> Sent from my iPhone
>>
>> On Nov 7, 2011, at 1:28 PM, chris hegarty<chris.hegarty at oracle.com>
>> wrote:
>>
>>> Hi Gary,
>>>
>>> Thanks for taking this bug.
>>>
>>> In similar tests I've preferred to use j.u.c.Paser/CountDownLatch to
>>> coordinate between threads. So, you could completely remove the
>>> sleep(1000) from the run method and use a
>>> Phaser.arriveAndAwaitAdvance(). This will guarantee both threads will
>>> reach a particular point before invoking stop on the group. That way,
>>> either thread can invoke stop and set the flag to prevent the other
>>> thread from invoking stop too ( if it were to get there! ).
>>>
>>> I agree with Rémi's comments about wait/notify. My comments (above)
>>> should work with well with his suggestion. Then no more nasty sleeps!
>>>
>>> Thanks,
>>> -Chris.
>>>
>>> On 07/11/2011 16:03, Gary Adams wrote:
>>>>
>>>>
>>>> Here's another test with race conditions built into the test
>>>> that can be easily avoided
>>>>
>>>> CR#7084033 - TEST_BUG: test/java/lang/ThreadGroup/Stop.java fails
>>>> intermittently
>>>>
>>>> There are at least two race conditions in the test as currently
>>>> written.
>>>> The test uses sleeps as a way to yield time to other threads to
>>>> complete
>>>> their
>>>> tasks, but this may not be sufficient on slower machines.
>>>>
>>>> The first race condition is in the starting of the test threads.
>>>> To ensure both threads are started, the run check for the first thread
>>>> should
>>>> also check that the second thread is not null.
>>>>
>>>> The second race condition in the main thread presumes that
>>>> the group stop has been issued within 3000 milliseconds.
>>>> A simple loop on the delay can be gated by a flag set after the group
>>>> stop has been issued.
>>>>
>>>> diff --git a/test/java/lang/ThreadGroup/Stop.java
>>>> b/test/java/lang/ThreadGroup/Stop.java
>>>> --- a/test/java/lang/ThreadGroup/Stop.java
>>>> +++ b/test/java/lang/ThreadGroup/Stop.java
>>>> @@ -32,6 +32,7 @@
>>>> private static Thread first=null;
>>>> private static Thread second=null;
>>>> private static ThreadGroup group = new ThreadGroup("");
>>>> + private static boolean groupStopped =false ;
>>>>
>>>> Stop() {
>>>> Thread thread = new Thread(group, this);
>>>> @@ -47,8 +48,11 @@
>>>> while (true) {
>>>> try {
>>>> Thread.sleep(1000); // Give other thread a chance to start
>>>> - if (Thread.currentThread() == first)
>>>> + if ((Thread.currentThread() == first)
>>>> +&& (second != null)) {
>>>> + groupStopped = true;
>>>> group.stop();
>>>> + }
>>>> } catch(InterruptedException e){
>>>> }
>>>> }
>>>> @@ -57,7 +61,10 @@
>>>> public static void main(String[] args) throws Exception {
>>>> for (int i=0; i<2; i++)
>>>> new Stop();
>>>> + do {
>>>> Thread.sleep(3000);
>>>> + } while (!groupStopped) ;
>>>> +
>>>> boolean failed = second.isAlive();
>>>> first.stop(); second.stop();
>>>> if (failed)
>>>>
>>>>
>>>>