jmx-dev RFR 8071487: javax/management/monitor/GaugeMonitorDeadlockTest.java timed out

classic Classic list List threaded Threaded
6 messages Options
Reply | Threaded
Open this post in threaded view
|

jmx-dev RFR 8071487: javax/management/monitor/GaugeMonitorDeadlockTest.java timed out

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

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-
Reply | Threaded
Open this post in threaded view
|

Re: jmx-dev RFR 8071487: javax/management/monitor/GaugeMonitorDeadlockTest.java timed out

David Holmes
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". ???

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-
Reply | Threaded
Open this post in threaded view
|

Re: jmx-dev RFR 8071487: javax/management/monitor/GaugeMonitorDeadlockTest.java timed out

Jaroslav Bachorik
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

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

Reply | Threaded
Open this post in threaded view
|

Re: jmx-dev RFR 8071487: javax/management/monitor/GaugeMonitorDeadlockTest.java timed out

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

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-
>
Reply | Threaded
Open this post in threaded view
|

Re: jmx-dev RFR 8071487: javax/management/monitor/GaugeMonitorDeadlockTest.java timed out

Jaroslav Bachorik
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-
>>

Reply | Threaded
Open this post in threaded view
|

Re: jmx-dev RFR 8071487: javax/management/monitor/GaugeMonitorDeadlockTest.java timed out

David Holmes
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-
>>>
>