RFR(S): 6952105 TEST_BUG: testcase failure, not very often, com/sun/jdi/SuspendThreadTest.java

[Staffan Larsen]

On 19 feb 2014, at 10:38, David Holmes <david.holmes at oracle.com> wrote:

> On 19/02/2014 7:01 PM, Staffan Larsen wrote:
>> Thanks for the feedback!
>> 
>> I chose to use yet another variable to avoid the spurious wakeups. I’ve
>> also increased the range of the synchronized statement to avoid the race.
>> 
>> http://cr.openjdk.java.net/~sla/6952105/webrev.01/
> 
> Slightly simpler to just do:
> 
> bkptSignal.wait(5000);
> if (!signalSent)
>  continue;
> 
> but what you have works.
> 
> Also signalSent doesn't need to be volatile as it is only accessed within the sync blocks.

True. And true for bkptCount as well now, except for one usage in a println. I’ll remove the volatile on signalSent, but keep it on bkptCount.

Thanks,
/Staffan

> 
> Thanks,
> David
> 
>> Thanks,
>> /Staffan
>> 
>> On 19 feb 2014, at 07:09, David Holmes <david.holmes at oracle.com> wrote:
>> 
>>> On 19/02/2014 7:17 AM, shanliang wrote:
>>>> I am looking at the old file:
>>>> 143         while (bkptCount < maxBkpts) {
>>>> 144             prevBkptCount = bkptCount;
>>>> 
>>>> suppose the following execution sequence:
>>>> 1)   when Line 143 was called by Thread1, we had bkptCount ==
>>>> maxBkpts - 1;
>>>> 
>>>> 2) bkptCount++ was executed by thread2;
>>>> 
>>>> 3) Line 144 was called by thread1,
>>>> 
>>>> in this case it was sure that the line
>>>>   152                 failure("failure: test hung");
>>>> would be called.
>>> 
>>> Yes I was looking at that race too. The comments suggest that we
>>> should never reach a point where we get to maxBkpts, so this failure
>>> would be very rare and would likely indicate a real problem.
>>> 
>>>> It is good to add:
>>>>   synchronized (bkptSignal)
>>>> in the fix, but we need to put Line 143 and 144 into synchronization too.
>>>> 
>>>> To deal with a spurious wakeup, we might do like this:
>>>>       long stopTime = System.currentTimeMillis() + 5000;
>>>>       do {
>>>>           try {
>>>>               bkptSignal.wait(100);
>>>>           } catch (InterruptedException e){}
>>>>       } while(prevBkptCount == bkptCount && System.currentTimeMillis()
>>>> < stopTime);
>>> 
>>> It is better to use System.nanoTime() rather than the non-monotonic
>>> currentTimeMillis(). And you really want a while loop rather than
>>> do-while so we don't always do that 100ms wait.
>>> 
>>> David
>>> 
>>>> Shanliang
>>>> 
>>>> David Holmes wrote:
>>>>> On 18/02/2014 11:03 PM, Staffan Larsen wrote:
>>>>>> 
>>>>>> On 18 feb 2014, at 13:09, David Holmes <david.holmes at oracle.com> wrote:
>>>>>> 
>>>>>>> Hi Staffan,
>>>>>>> 
>>>>>>> If you get a spurious wakeup from wait():
>>>>>>> 
>>>>>>> 151             try {
>>>>>>> 152                 synchronized (bkptSignal) {
>>>>>>> 153                     bkptSignal.wait(5000);
>>>>>>> 154                 }
>>>>>>> 155             } catch (InterruptedException ee) {
>>>>>>> 156             }
>>>>>>> 157             if (prevBkptCount == bkptCount) {
>>>>>>> 158                 failure("failure: test hung");
>>>>>>> 
>>>>>>> you could report failure. But that is far less likely than the
>>>>>>> current problem using sleep.
>>>>>> 
>>>>>> Right. Adding “continue;” inside the catch(InterruptedException)
>>>>>> block should guard against that.
>>>>> 
>>>>> No, a spurious wakeup is not an interrupt - the wait() will simply
>>>>> return.
>>>>> 
>>>>> David
>>>>>> 
>>>>>> /Staffan
>>>>>> 
>>>>>>> 
>>>>>>> David
>>>>>>> 
>>>>>>> On 18/02/2014 8:19 PM, Staffan Larsen wrote:
>>>>>>>> Still looking for Reviewer for this change.
>>>>>>>> 
>>>>>>>> Thanks,
>>>>>>>> /Staffan
>>>>>>>> 
>>>>>>>> On 11 feb 2014, at 15:12, Staffan Larsen
>>>>>>>> <staffan.larsen at oracle.com> wrote:
>>>>>>>> 
>>>>>>>>> Updated the test to use proper synchronization and notification
>>>>>>>>> between threads. Should be more stable and much faster.
>>>>>>>>> 
>>>>>>>>> bug: https://bugs.openjdk.java.net/browse/JDK-6952105
>>>>>>>>> webrev: http://cr.openjdk.java.net/~sla/6952105/webrev.00/
>>>>>>>>> 
>>>>>>>>> Thanks,
>>>>>>>>> /Staffan
>>