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

[Staffan Larsen]

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/

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

-------------- next part --------------
An HTML attachment was scrubbed...
URL: http://mail.openjdk.java.net/pipermail/serviceability-dev/attachments/20140219/352fb18b/attachment-0001.html