jmx-dev Codereview request for JDK-8023669: MBean*Info.hashCode : NPE

[Jaroslav Bachorik]

On 09/11/2013 09:54 AM, shanliang wrote:
> Daniel Fuchs wrote:
>> On 9/11/13 2:05 AM, David Holmes wrote:
>>> On 30/08/2013 12:11 AM, shanliang wrote:
>>>> Here is the new version:
>>>>     http://cr.openjdk.java.net/~sjiang/jdk-8023669/01/
>>>
>>> This seems good to address the NPE potential.
>>>
>>> You are changing the values of the hashcodes though - is that a problem?
>> Hi David,
>>
>> The algorithm by which hashCode() is computed isn't specified - so it
>> shouldn't
>> be an issue changing it - should it?
>>
>> I must say I don't know how HashMap is serialized - for instance. So
>> assuming
>> we have an App1 sending a Map of MBeanInfos to an App2 over the wire,
>> is that
>> a problem if in App2 the MBeanInfo hashCodes resolve to different values?
>> I assume it doesn't...
> If app1 sends a Map<MBeanInfo, Object> to app2, the hashCode of each key
> (MBeanInfo) will be re-calculated in app2, it is because the variable
> "hashCode" of an MBeanOInfo is declared as "transient", the value
> calculated in the app1 is not serialized and sent to the app2. So must
> no problem here.
> 
> An exception is that we have Map<int, Object>, and the key "int" here is
> hashCode of an MBeanInfo, if app1 sends this map to app2, then app2 uses
> an MBenInfo's hashCode to look for a value in the map. But this seems
> little possible.

Storing objects in a map by their hashcodes makes little sense. Two
distinct objects can share the same hashcode easily - thus this kind of
usage would mess up the application anyway.

-JB-

> 
> Thanks,
> Shanliang
>>
>> I have never considered values of hashCode() to be part of the
>> standard/API.
>> Since the algorithm isn't specified, then 2 implementations of the
>> standard should
>> be free to use different algorithms - and this shouldn't prevent then to
>> interoperate. So I hope we're safe in modifying the hashCode algorithm.
>>
>> The important part is that it still satisfies the hashCode/equals
>> contract.
>>
>> cheers,
>>
>> -- daniel
>>
>>>
>>> In javax/management/MBeanInfo.java
>>>
>>> Objects.hash(getClassName(), getDescriptor().hashCode())
>>>
>>> should, I think, be
>>>
>>> Objects.hash(getClassName(), getDescriptor())
>>> David
>>> -----
>>>
>>>> Indeed, calling Objects.hash(Object ...) is a good idea, which
>>>> simplifies the code.
>>>>
>>>> I used also Arrays.hashCode() to simplify the code,  now the fix likes
>>>> really simple.
>>>>
>>>> I have passed JCK tests, unit tests of management are passed too in my
>>>> desk.
>>>>
>>>> toString() worked perfectly in the test, nothing to fix.
>>>>
>>>> Shanliang
>>>>
>>>> Daniel Fuchs wrote:
>>>>> On 8/29/13 9:34 AM, shanliang wrote:
>>>>>> Hi,
>>>>>>
>>>>>> Please review following fix, it addresses the issue only in the
>>>>>> method
>>>>>> "hashCode":
>>>>>>     bug: https://bugs.openjdk.java.net/browse/JDK-8023669
>>>>>>     webrev: http://cr.openjdk.java.net/~sjiang/jdk-8023669/00/
>>>>>>
>>>>>> Thanks,
>>>>>> Shanliang
>>>>>>
>>>>>
>>>>> Hi Shanliang,
>>>>>
>>>>> <http://cr.openjdk.java.net/~sjiang/jdk-8023669/00/src/share/classes/javax/management/MBeanAttributeInfo.java.frames.html>
>>>>>
>>>>>
>>>>>
>>>>> I suggest to simplify this by calling:
>>>>>
>>>>> public int hashCode() {
>>>>>    return Objects.hash(getName(), getType());
>>>>> }
>>>>>
>>>>> (see
>>>>> <http://docs.oracle.com/javase/7/docs/api/java/util/Objects.html#hash%28java.lang.Object...%29>)
>>>>>
>>>>>
>>>>>
>>>>> <http://cr.openjdk.java.net/~sjiang/jdk-8023669/00/src/share/classes/javax/management/MBeanConstructorInfo.java.frames.html>
>>>>>
>>>>>
>>>>>
>>>>>     int hash = getName() == null ? 10 : getName().hashCode();
>>>>>
>>>>> could be replaced by:
>>>>>
>>>>>     int hash = Objects.hashCode(getName());
>>>>>
>>>>> Generally - and that stands for the other files you modified, you can
>>>>> simplify things by replacing  x.hashCode() with Objects.hashCode(x)
>>>>> whenever there's the possibility that x can be null.
>>>>>
>>>>> Note however that Objects is an API @since JDK 7 - so if you intend
>>>>> to backport this fix to 6 & 5 you will need to alter your changeset
>>>>> when backporting it.
>>>>>
>>>>>
>>>>> MBeanOperationInfo.java, MBeanParameterInfo.java: I suggest
>>>>> to use Objects.hash(...);
>>>>>
>>>>> best regards,
>>>>>
>>>>> -- daniel
>>>>>
>>>>> BTW: one more question: you're also testing toString() in the test
>>>>>      and that's good - but are there any toString() that will require
>>>>>      fixing?
>>>>>
>>>>>
>>>>
>>
>