jmx-dev Codereview: JDK-8065764 javax/management/monitor/CounterMonitorTest.java hangs

[shanliang]

Jaroslav Bachorik wrote:
> On 12/02/2014 02:40 PM, shanliang wrote:
>> Jaroslav Bachorik wrote:
>>> On 12/01/2014 02:50 PM, shanliang wrote:
>>>> Hi,
>>>> please review this test bug fix:
>>>>
>>>> webrev:
>>>> http://cr.openjdk.java.net/~sjiang/JDK-8065764/00/
>>>>
>>>> bug:
>>>> https://bugs.openjdk.java.net/browse/JDK-8065764
>>>>
>>>
>>> test/javax/management/monitor/CounterMonitorTest.java
>>> L61 - observedValue could be Integer
>> Could be, but not make difference, observedValue is only used with the
>> operation "==", like
>>     while (value != observedValue)
>
> In that case having a correct type would make even more sense for the 
> readability.
Agree in general:) but the variable "count" is declared as "Object" in 
StdObservedObject, and "observedValue" is used to save observed "count" 
value, so better to keep the same type for "more readability"? and avoid 
a casting when setting "observedValue"
>
>>> L225-238 - you could replace this block with the usage of Phaser (if
>>> you'd do that you could completely remove 'observedValue')
>> Not sure that it is a good idea to remove "observedValue" by using
>> Phaser, yes using Phaser can tell when the monitor does an observation,
>> but it is better to know which count value the monitor observed, in case
>> the thread was waked up accidentally.
>
> I don't think phaser can be confused by spurious wake-ups.
>
> If you want to keep the current way of synchronizing then you should 
> make the 'count' variable volatile (L77) - it might be read and 
> written to from different threads.
Phaser should not make a false wakeup, but use "observedValue" can make 
sure that the observation happens on the right "count" value.
Yes "count" should be declared as "volatile".

Here is the new version:
    http://cr.openjdk.java.net/~sjiang/JDK-8065764/01/

Thanks,
Shanliang

>
> -JB-
>
>>
>> Thanks,
>> Shanliang
>>>
>>> -JB-
>>>
>>>
>>>> The test tested the mode "difference", according to the Javadoc:
>>>>       If the counter difference mode is used, the value of the derived
>>>> gauge is calculated as the difference between the observed counter
>>>> values for two successive observations.
>>>>
>>>> The test set the first value and then waited 2 times of
>>>> granularityperiod at line 171, hoped that the monitor would get the
>>>> first observation during this waiting time, but the test could fail
>>>> because granularityperiod * 2 was not enough and the test did the 
>>>> second
>>>> set before the monitor did the first observation.
>>>>
>>>> It is easy to make the test timeout by commenting out the line 171.
>>>>
>>>> The proposed solution is to get informed when the monitor did
>>>> observation on calling:
>>>>     StdObservedObject.getNbObjects();
>>>>
>>>> Thanks,
>>>> Shanliang
>>>>
>>>>
>>>
>>
>