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

List:       openjdk-serviceability-dev
Subject:    Re: RFR 8071487: javax/management/monitor/GaugeMonitorDeadlockTest.java timed out
From:       David Holmes <david.holmes () oracle ! com>
Date:       2015-06-26 6:42:39
Message-ID: 558CF45F.7060200 () oracle ! com
[Download RAW message or body]

Looks good!

Thanks,
David

On 25/06/2015 7:53 PM, Jaroslav Bachorik wrote:
> On 24.6.2015 11:22, David Holmes wrote:
>> On 23/06/2015 5:33 PM, Jaroslav Bachorik wrote:
>>> Hi David,
>>>
>>> On 23.6.2015 08:04, David Holmes wrote:
>>>> On 23/06/2015 1:36 AM, Jaroslav Bachorik wrote:
>>>>> Please, review the following test change
>>>>>
>>>>> Issue : https://bugs.openjdk.java.net/browse/JDK-8071487
>>>>> Webrev: http://cr.openjdk.java.net/~jbachorik/8071487/webrev.00
>>>>>
>>>>> GaugeMonitorDeadlockTest fails intermittently due data race - the call
>>>>> to monitorProxy.start() on L105 will eventually result in incrementing
>>>>> the GetCount attribute. If that happens before L109 had the chance to
>>>>> run the loop on L113-128 will become infinite. The initial value will
>>>>> contain the already incremented GetCount value and GetCount attribute
>>>>> value will not get further incremented.
>>>>
>>>> The reordering of the start() and initial observedProxy.getGetCount
>>>> seems reasonable.
>>>>
>>>> The changes to the timeout handling less so. The alarm code doesn't
>>>> look
>>>> right to me. Each time you call check(true) in the loop you advance the
>>>> time when the alarm "goes off". ???
>>>
>>> Stupid me :( Thanks for catching this.
>>>
>>> http://cr.openjdk.java.net/~jbachorik/8071487/webrev.01
>>
>> That looks better, but the Alarm class would benefit from some simple
>> descriptive comments. I must admit I don't really see the need for
>> introducing it.
>
> I expected to reuse it in some of the other JMX monitoring tests. But it
> didn't turn out that way.
>
> I've removed the Alarm class from the test and just switched to using
> Util.adjustTimeout() instead of doing the parsing of
> "test.timeout.factor" system property by hand.
>
> http://cr.openjdk.java.net/~jbachorik/8071487/webrev.02
>
> Thanks,
>
> -JB-
>
>>
>> Thanks,
>> David
>>
>>> -JB-
>>>
>>>>
>>>> David
>>>>
>>>>
>>>>
>>>>> I took the liberty to fix the same issue in
>>>>> test/javax/management/monitor/StringMonitorDeadlockTest.java, not
>>>>> waiting for the real test failure.
>>>>>
>>>>> Thanks,
>>>>>
>>>>> -JB-
>>>
>
[prev in list] [next in list] [prev in thread] [next in thread]