jmx-dev RFR 6309226: TEST: java/lang/management/ThreadMXBean/SynchronizationStatistics.java didn't check Thread.sleep

[David Holmes]

Sorry for the delay - still catching up after vacation (only 435 openjdk 
emails left :( ).

On 9/12/2013 9:46 PM, Jaroslav Bachorik wrote:
> On 19.11.2013 19:55, David Holmes wrote:
>> On 20/11/2013 12:43 AM, Jaroslav Bachorik wrote:
>>> Hi David,
>>>
>>> On 18.11.2013 22:03, David Holmes wrote:
>>>> Hi Jaroslav,
>>>>
>>>> I think your phaser usage is incorrect:
>>>>
>>>>   88             public void run() {
>>>>    89                 p.arriveAndAwaitAdvance(); // phase[1]
>>>>    90                 synchronized(lock1) {
>>>>    91                     System.out.println("[LockerThread obtained
>>>> Lock1]");
>>>>    92                     p.arrive(); // phase[2]
>>>>    93                 }
>>>>    94                 p.arriveAndAwaitAdvance(); // phase[3]
>>>>    95             }
>>>>
>>>> Here the current thread can release itself at line 94. You have assumed
>>>> that the phase[2] arrive will be a trigger to release the main thread
>>>> but it may not have reached its arriveAndAwaitAdvance phase[2]
>>>> statement
>>>> yet, so the current thread arrives then does arrive-and-wait but the
>>>> number of arrivals is 2 so it doesn't wait.
>>>>
>>>> A CyclicBarrier per phase may be clearer.
>>>
>>> Yes, thanks for pointing this out. But, wouldn't
>>> p.arriveAndAwaitAdvance() instead of p.arrive() suffice? I've tried to
>>> replace Phaser with CyclicBarrier but while it seems to work as expected
>>> it pollutes code with the necessity to catch various exceptions
>>> (InterruptedException, BarrierException etc.). So, if the simple fix
>>> with Phaser would do the trick I would better use that.
>>
>> In this case yes arriveAndAwaitAdvance would fix it. I don't know if
>> other tests have the same issue - the concern would be ensuring there's
>> no possibility of deadlocks in general.
>
> I've updated the webrev with the one using p.arriveAndAwaitAdvance() at
> line 92 - http://cr.openjdk.java.net/~jbachorik/6309226/webrev.04
>
> Are you ok with this change then?

I think you have the same problem anywhere that arrive() is used eg:

  129                     p.arrive(); // phase[2]
  130                     p.arriveAndAwaitAdvance(); // phase[3]

and

  185                     p.arrive(); // phase[2]
  186                 }
  187                 p.arriveAndAwaitAdvance(); // phase[3]

David
------

> Thanks,
>
> -JB-
>
>
>>
>> Thanks,
>> David
>>
>>> -JB-
>>>
>>>>
>>>> David
>>>>
>>>> On 18/11/2013 7:59 PM, Jaroslav Bachorik wrote:
>>>>> Hi,
>>>>>
>>>>> after discussing this with Mandy I've rewritten the test to use the
>>>>> j.u.concurrent for synchronization - this also makes it much easier to
>>>>> follow the test logic.
>>>>>
>>>>> The waited time, the blocked time and the waited counts are only
>>>>> checked
>>>>> for sanity (increasing values) since it is not possible to do the
>>>>> reliable checks against hard numbers.
>>>>>
>>>>> I ran the test in a tight loop for 1500 times using -Xcomp and -Xint
>>>>> and
>>>>> the test seems to pass constantly.
>>>>>
>>>>> New webrev: http://cr.openjdk.java.net/~jbachorik/6309226/webrev.03
>>>>>
>>>>> Thanks,
>>>>>
>>>>> -JB-
>>>>>
>>>>>
>>>>> On 21.10.2013 13:55, Jaroslav Bachorik wrote:
>>>>>> Please, review this small patch for a test failing due to the updated
>>>>>> implementation in JDK6.
>>>>>>
>>>>>> Issue:  https://bugs.openjdk.java.net/browse/JDK-6309226
>>>>>> Webrev: http://cr.openjdk.java.net/~jbachorik/6309226/webrev.00/
>>>>>>
>>>>>> The test fails due to the change in mustang where
>>>>>> ThreadMXBean.getThreadInfo().getWaitedTime() and
>>>>>> ThreadMXBean.getThreadInfo().getWaitedCount() include Thread.sleep()
>>>>>> too. Unfortunately, Thread.sleep() is used throughout the test for
>>>>>> synchronization purposes and this breaks the test.
>>>>>>
>>>>>> In the patch I propose to replace Thread.sleep() with busy wait and
>>>>>> hinting the scheduler by Thread.yield(). While not very elegant it
>>>>>> successfully works around inclusion of unknown number of
>>>>>> Thread.sleep()s
>>>>>> (they are called in loop).
>>>>>>
>>>>>> Thanks,
>>>>>>
>>>>>> -JB-
>>>>>
>>>
>