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

[Jaroslav Bachorik]

On 12/02/2014 07:17 PM, shanliang wrote:
> Jaroslav Bachorik wrote:
>> On 12/02/2014 06:56 PM, shanliang wrote:
>>> 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.
>>
>> I still don't see how using Object instead of Integer type could check
>> this functionality. But if you insist on keeping it this way I won't
>> block fixing this failing test any further. Reviewed.
> You can simply pass an Object instead of an Integer to NbObjects, and
> see what happen. The monitor does type check only at monitoring time.

You can but you are passing Integer instance which are just dressed as 
Objects anyway :)

-JB-

>
> Thanks for review.
> Shanliang
>>
>> -JB-
>>
>>>
>>> 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
>>>>>>>>>>>
>>>>>>>>>>>
>>>>>>>>>>
>>>>>>>>>
>>>>>>>>
>>>>>>>
>>>>>>
>>>>>
>>>>
>>>
>>
>