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