jmx-dev RFR 8139870: sun.management.LazyCompositeData.isTypeMatched() fails for composite types with items of ArrayType

classic Classic list List threaded Threaded
7 messages Options
Reply | Threaded
Open this post in threaded view
|

jmx-dev RFR 8139870: sun.management.LazyCompositeData.isTypeMatched() fails for composite types with items of ArrayType

Jaroslav Bachorik
Please, review the following change

Issue : https://bugs.openjdk.java.net/browse/JDK-8139870
Webrev: http://cr.openjdk.java.net/~jbachorik/8139870/webrev.00

sun.management.LazyCompositeData.isTypeMatched() method is used to
compare two composite types with the backward compatibility in mind. The
idea is that when we have two types (type1, type2) type1 is matched by
type2 when and only when type2 contains all the items of type1 and their
types are in turn matching the item types from type1, recursively.

However, this method fails to account for ArrayType type and instead of
calling isTypeMatched() recursively on the array type it performs plain
Object.equals(). This will cause problems when one tries to safely
evolve (only adding items) composite types referred through ArrayType items.

The patch adds the missing functionality.

In http://cr.openjdk.java.net/~jbachorik/8139870/webrev.00/cleanup there
is a followup webrev of code warnings cleanup for s.m.LazyComponentData.

Thanks,

-JB-
Reply | Threaded
Open this post in threaded view
|

Re: jmx-dev RFR 8139870: sun.management.LazyCompositeData.isTypeMatched() fails for composite types with items of ArrayType

Daniel Fuchs
Hi Jaroslav,

This - and the cleanup - look good to me, but it would
be nicer if it was accompanied by a unit test :-)

best regards,

-- daniel

On 19/10/15 13:37, Jaroslav Bachorik wrote:

> Please, review the following change
>
> Issue : https://bugs.openjdk.java.net/browse/JDK-8139870
> Webrev: http://cr.openjdk.java.net/~jbachorik/8139870/webrev.00
>
> sun.management.LazyCompositeData.isTypeMatched() method is used to
> compare two composite types with the backward compatibility in mind. The
> idea is that when we have two types (type1, type2) type1 is matched by
> type2 when and only when type2 contains all the items of type1 and their
> types are in turn matching the item types from type1, recursively.
>
> However, this method fails to account for ArrayType type and instead of
> calling isTypeMatched() recursively on the array type it performs plain
> Object.equals(). This will cause problems when one tries to safely
> evolve (only adding items) composite types referred through ArrayType
> items.
>
> The patch adds the missing functionality.
>
> In http://cr.openjdk.java.net/~jbachorik/8139870/webrev.00/cleanup there
> is a followup webrev of code warnings cleanup for s.m.LazyComponentData.
>
> Thanks,
>
> -JB-

Reply | Threaded
Open this post in threaded view
|

Re: jmx-dev RFR 8139870: sun.management.LazyCompositeData.isTypeMatched() fails for composite types with items of ArrayType

Jaroslav Bachorik
On 19.10.2015 14:40, Daniel Fuchs wrote:
> Hi Jaroslav,
>
> This - and the cleanup - look good to me, but it would
> be nicer if it was accompanied by a unit test :-)

This time with test -
http://cr.openjdk.java.net/~jbachorik/8139870/webrev.01

Testing *only* the correctness of the newly introduced functionality
(eg. the test fails on older JDKs and passes on the build with the fix
in place). Providing full coverage for LazyCompositeData is probably out
of the scope of this change.

-JB-

>
> best regards,
>
> -- daniel
>
> On 19/10/15 13:37, Jaroslav Bachorik wrote:
>> Please, review the following change
>>
>> Issue : https://bugs.openjdk.java.net/browse/JDK-8139870
>> Webrev: http://cr.openjdk.java.net/~jbachorik/8139870/webrev.00
>>
>> sun.management.LazyCompositeData.isTypeMatched() method is used to
>> compare two composite types with the backward compatibility in mind. The
>> idea is that when we have two types (type1, type2) type1 is matched by
>> type2 when and only when type2 contains all the items of type1 and their
>> types are in turn matching the item types from type1, recursively.
>>
>> However, this method fails to account for ArrayType type and instead of
>> calling isTypeMatched() recursively on the array type it performs plain
>> Object.equals(). This will cause problems when one tries to safely
>> evolve (only adding items) composite types referred through ArrayType
>> items.
>>
>> The patch adds the missing functionality.
>>
>> In http://cr.openjdk.java.net/~jbachorik/8139870/webrev.00/cleanup there
>> is a followup webrev of code warnings cleanup for s.m.LazyComponentData.
>>
>> Thanks,
>>
>> -JB-
>

Reply | Threaded
Open this post in threaded view
|

Re: jmx-dev RFR 8139870: sun.management.LazyCompositeData.isTypeMatched() fails for composite types with items of ArrayType

Daniel Fuchs
On 19/10/15 15:36, Jaroslav Bachorik wrote:
> On 19.10.2015 14:40, Daniel Fuchs wrote:
>> Hi Jaroslav,
>>
>> This - and the cleanup - look good to me, but it would
>> be nicer if it was accompanied by a unit test :-)
>
> This time with test -
> http://cr.openjdk.java.net/~jbachorik/8139870/webrev.01

Thanks for the test Jaroslav!

> Testing *only* the correctness of the newly introduced functionality
> (eg. the test fails on older JDKs and passes on the build with the fix
> in place). Providing full coverage for LazyCompositeData is probably out
> of the scope of this change.

Yes, I agree. This is fine.
BTW - did you verify that the JCK still passes?
Otherwise you might need to challenge it...

best regards,

-- daniel

>
> -JB-
>
>>
>> best regards,
>>
>> -- daniel
>>
>> On 19/10/15 13:37, Jaroslav Bachorik wrote:
>>> Please, review the following change
>>>
>>> Issue : https://bugs.openjdk.java.net/browse/JDK-8139870
>>> Webrev: http://cr.openjdk.java.net/~jbachorik/8139870/webrev.00
>>>
>>> sun.management.LazyCompositeData.isTypeMatched() method is used to
>>> compare two composite types with the backward compatibility in mind. The
>>> idea is that when we have two types (type1, type2) type1 is matched by
>>> type2 when and only when type2 contains all the items of type1 and their
>>> types are in turn matching the item types from type1, recursively.
>>>
>>> However, this method fails to account for ArrayType type and instead of
>>> calling isTypeMatched() recursively on the array type it performs plain
>>> Object.equals(). This will cause problems when one tries to safely
>>> evolve (only adding items) composite types referred through ArrayType
>>> items.
>>>
>>> The patch adds the missing functionality.
>>>
>>> In http://cr.openjdk.java.net/~jbachorik/8139870/webrev.00/cleanup there
>>> is a followup webrev of code warnings cleanup for s.m.LazyComponentData.
>>>
>>> Thanks,
>>>
>>> -JB-
>>
>

Reply | Threaded
Open this post in threaded view
|

Re: jmx-dev RFR 8139870: sun.management.LazyCompositeData.isTypeMatched() fails for composite types with items of ArrayType

Jaroslav Bachorik
On 19.10.2015 16:28, Daniel Fuchs wrote:

> On 19/10/15 15:36, Jaroslav Bachorik wrote:
>> On 19.10.2015 14:40, Daniel Fuchs wrote:
>>> Hi Jaroslav,
>>>
>>> This - and the cleanup - look good to me, but it would
>>> be nicer if it was accompanied by a unit test :-)
>>
>> This time with test -
>> http://cr.openjdk.java.net/~jbachorik/8139870/webrev.01
>
> Thanks for the test Jaroslav!
>
>> Testing *only* the correctness of the newly introduced functionality
>> (eg. the test fails on older JDKs and passes on the build with the fix
>> in place). Providing full coverage for LazyCompositeData is probably out
>> of the scope of this change.
>
> Yes, I agree. This is fine.
> BTW - did you verify that the JCK still passes?
> Otherwise you might need to challenge it...

Not yet. I really hope it will pass - otherwise it would mean that the
incorrect behaviour has been baked in :/

-JB-

>
> best regards,
>
> -- daniel
>
>>
>> -JB-
>>
>>>
>>> best regards,
>>>
>>> -- daniel
>>>
>>> On 19/10/15 13:37, Jaroslav Bachorik wrote:
>>>> Please, review the following change
>>>>
>>>> Issue : https://bugs.openjdk.java.net/browse/JDK-8139870
>>>> Webrev: http://cr.openjdk.java.net/~jbachorik/8139870/webrev.00
>>>>
>>>> sun.management.LazyCompositeData.isTypeMatched() method is used to
>>>> compare two composite types with the backward compatibility in mind.
>>>> The
>>>> idea is that when we have two types (type1, type2) type1 is matched by
>>>> type2 when and only when type2 contains all the items of type1 and
>>>> their
>>>> types are in turn matching the item types from type1, recursively.
>>>>
>>>> However, this method fails to account for ArrayType type and instead of
>>>> calling isTypeMatched() recursively on the array type it performs plain
>>>> Object.equals(). This will cause problems when one tries to safely
>>>> evolve (only adding items) composite types referred through ArrayType
>>>> items.
>>>>
>>>> The patch adds the missing functionality.
>>>>
>>>> In http://cr.openjdk.java.net/~jbachorik/8139870/webrev.00/cleanup
>>>> there
>>>> is a followup webrev of code warnings cleanup for
>>>> s.m.LazyComponentData.
>>>>
>>>> Thanks,
>>>>
>>>> -JB-
>>>
>>
>

Reply | Threaded
Open this post in threaded view
|

Re: jmx-dev RFR 8139870: sun.management.LazyCompositeData.isTypeMatched() fails for composite types with items of ArrayType

Jaroslav Bachorik
On 19.10.2015 16:31, Jaroslav Bachorik wrote:

> On 19.10.2015 16:28, Daniel Fuchs wrote:
>> On 19/10/15 15:36, Jaroslav Bachorik wrote:
>>> On 19.10.2015 14:40, Daniel Fuchs wrote:
>>>> Hi Jaroslav,
>>>>
>>>> This - and the cleanup - look good to me, but it would
>>>> be nicer if it was accompanied by a unit test :-)
>>>
>>> This time with test -
>>> http://cr.openjdk.java.net/~jbachorik/8139870/webrev.01
>>
>> Thanks for the test Jaroslav!
>>
>>> Testing *only* the correctness of the newly introduced functionality
>>> (eg. the test fails on older JDKs and passes on the build with the fix
>>> in place). Providing full coverage for LazyCompositeData is probably out
>>> of the scope of this change.
>>
>> Yes, I agree. This is fine.
>> BTW - did you verify that the JCK still passes?
>> Otherwise you might need to challenge it...
>
> Not yet. I really hope it will pass - otherwise it would mean that the
> incorrect behaviour has been baked in :/

Ran JCK, just to be sure. It did pass, as expected (I am changing the
internal sun.management implementation and not a Java SE API, afterall).

-JB-

>
> -JB-
>
>>
>> best regards,
>>
>> -- daniel
>>
>>>
>>> -JB-
>>>
>>>>
>>>> best regards,
>>>>
>>>> -- daniel
>>>>
>>>> On 19/10/15 13:37, Jaroslav Bachorik wrote:
>>>>> Please, review the following change
>>>>>
>>>>> Issue : https://bugs.openjdk.java.net/browse/JDK-8139870
>>>>> Webrev: http://cr.openjdk.java.net/~jbachorik/8139870/webrev.00
>>>>>
>>>>> sun.management.LazyCompositeData.isTypeMatched() method is used to
>>>>> compare two composite types with the backward compatibility in mind.
>>>>> The
>>>>> idea is that when we have two types (type1, type2) type1 is matched by
>>>>> type2 when and only when type2 contains all the items of type1 and
>>>>> their
>>>>> types are in turn matching the item types from type1, recursively.
>>>>>
>>>>> However, this method fails to account for ArrayType type and
>>>>> instead of
>>>>> calling isTypeMatched() recursively on the array type it performs
>>>>> plain
>>>>> Object.equals(). This will cause problems when one tries to safely
>>>>> evolve (only adding items) composite types referred through ArrayType
>>>>> items.
>>>>>
>>>>> The patch adds the missing functionality.
>>>>>
>>>>> In http://cr.openjdk.java.net/~jbachorik/8139870/webrev.00/cleanup
>>>>> there
>>>>> is a followup webrev of code warnings cleanup for
>>>>> s.m.LazyComponentData.
>>>>>
>>>>> Thanks,
>>>>>
>>>>> -JB-
>>>>
>>>
>>
>

Reply | Threaded
Open this post in threaded view
|

Re: jmx-dev RFR 8139870: sun.management.LazyCompositeData.isTypeMatched() fails for composite types with items of ArrayType

Daniel Fuchs
On 10/20/15 10:00 AM, Jaroslav Bachorik wrote:
> Ran JCK, just to be sure. It did pass, as expected (I am changing the
> internal sun.management implementation and not a Java SE API, afterall).
Thanks Jaroslav!

Looks good to me then :-)

-- daniel