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

[shanliang]

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.

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