[prev in list] [next in list] [prev in thread] [next in thread] 

List:       openjdk-serviceability-dev
Subject:    Re: RFR(S): 6952105 TEST_BUG: testcase failure, not very often, com/sun/jdi/SuspendThreadTest.java
From:       Staffan Larsen <staffan.larsen () oracle ! com>
Date:       2014-02-19 9:42:51
Message-ID: 626ADA78-77A9-44C3-B2D5-C42798C4F46F () oracle ! com
[Download RAW message or body]


On 19 feb 2014, at 10:38, David Holmes <david.holmes@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@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@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@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
> > 


[prev in list] [next in list] [prev in thread] [next in thread]