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

[shanliang]

Jaroslav Bachorik wrote:
> On 12/02/2014 05:22 PM, shanliang wrote:
>> Jaroslav Bachorik wrote:
>>> On 12/02/2014 04:22 PM, shanliang wrote:
>>>> 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"
>>>
>>> Any reason 'count' is declared as Object? The value is effectively an
>>> integer value - only integer values are ever assigned to it. All the
>>> operations silently suppose this would be an integer and yet the type
>>> information is thrown away and the data is stored as Object. Feels
>>> very strange ...
>> I think a possible reason is that the CounterMonitor supports only
>> Number type for comparing, and it accepts any type but detects whether
>> the observed variable could be "matched" to Number, pass an Object type
>> verifies this capability.
>
> Hm, could you direct me to the code doing this? 'setAttribute' method 
> seems to be inherited from 'Monitor' class and does not do any 
> additional checks. Then 'Monitor.monitor()' uses 'getAttribute' 
> (again, not overridden) to get back to the attribute :(
ConterMonitor overrides Monitor.isComparableTypeValid(...)

This method will be called by Monitor at the monitoring time and a 
Notification with the type "jmx.monitor.error.type" will be sent out if 
the observed attribute is not of type "Integer".

Yes the monitor might have to do type check with the method:
    Monitor.setObservedAttribute("NbObjects");

but the Javadoc declares only:
    Throws: IllegalArgumentException - The specified attribute is null.

maybe it is not sure that a monitor could do type check at setting time, 
for example if the MBeanInfo is null and getAttributeValue() returns null.

Shanliang
>
> Thanks,
>
> -JB-
>
>>>
>>>>>
>>>>>>> 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/
>>>
>>> Still using 'derivedGange' instead of 'derivedGauge' :(
>> Oh I will change it before pushing, if no other modification.
>>
>> Thanks,
>> Shanliang
>>>
>>> -JB-
>>>
>>>>
>>>> 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
>>>>>>>>
>>>>>>>>
>>>>>>>
>>>>>>
>>>>>
>>>>
>>>
>>
>