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

Staffan Larsen staffan.larsen at oracle.com
Wed Feb 19 01:42:51 PST 2014

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

More information about the serviceability-dev mailing list