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

[David Holmes]

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.

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
>