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

[David Holmes]

On 11/09/2013 5:37 PM, 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?

It shouldn't :) But that was why I asked. Even though the exact hashcode 
is not part of the contract/spec we need to think about what impact it 
might have just in case we suddenly realize it is a problem somewhere. 
The main effect of the change would be on how maps store and traverse 
these objects. Nobody should be relying on such details of course, but 
there always seems to be someone who does.

Anyway I raised the question and it's been explored and I don't see any 
flashing red lights so no problem. :)

Reviewed.

Cheers,
David

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