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.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
