Re: [7u backport] RFR: 7122142: (ann) Race condition between isAnnotationPresent and getAnnotations

[Dalibor Topic]

Looks fine to me as well.

cheers,
dalibor topic

On 3/3/14 1:21 PM, dmeetry degrave wrote:
> Hi all,
> 
> I would like to ask someone with a reviewer status in jdk7u project to look 
> at these changes.
> 
> thanks,
> dmeetry
> 
> On 02/27/2014 05:44 PM, Joel Borggren-Franck wrote:
>> Hi,
>>
>> I looked at webrev.1. Looks good.
>>
>> cheers
>> /Joel
>>
>> On 2014-02-25, dmeetry degrave wrote:
>>> Thanks for looking at this, Peter!
>>>
>>> On 02/24/2014 04:42 PM, Peter Levart wrote:
 Hi Dmeetry,

 On 02/22/2014 01:22 PM, dmeetry degrave wrote:
> Hi all,
>
> I would like to ask for a review of combined back port for
> 7u-dev/7u80. The main goal is to have a fix for 7122142 in jdk7, it
> also integrates the changes from 8005232, 7185456, 8022721
>
> https://bugs.openjdk.java.net/browse/JDK-7122142
> https://bugs.openjdk.java.net/browse/JDK-8005232
> https://bugs.openjdk.java.net/browse/JDK-7185456
> https://bugs.openjdk.java.net/browse/JDK-8022721
>
> Original jdk8 changes:
>
> 7122142: http://hg.openjdk.java.net/jdk8/tl/jdk/rev/e4ce6502eac0
> 8005232: http://hg.openjdk.java.net/jdk8/tl/jdk/rev/1109bfff4e92
> 7185456: http://hg.openjdk.java.net/jdk8/tl/jdk/rev/ae03282ba501
> 8022721: http://hg.openjdk.java.net/jdk8/tl/jdk/rev/2281a7f79738
>
> back port:
>
> http://cr.openjdk.java.net/~dmeetry/7122142.8005232.7185456.8022721/webrev.0/
>
>
> Patches can't be applied cleanly, hence it was a manual back port,
> though the final result is equivalent to applying the patches in
> chronological order (8005232, 7185456, 7122142, 8022721) and applying
> all the relevant rejected parts

 It's good to see those patches being back-ported to 7u. By browsing the
 webrev, I don't see any obvious difference between the original patches
 and the backport.
>>>
>>> there shouldn't be any!
>>>
 Do you happen to remember in what part of code there
 were rejects so that you had to manually apply the changes?
>>>
>>> there were conflicts due to small difference between 7 and 8
>>> (copyrights, white spaces, @SuppressWarnings, Class,...).
>>>
>>> I copied all rejected parts and original patches here:
>>>
>>> http://cr.openjdk.java.net/~dmeetry/7122142.8005232.7185456.8022721/webrev.1/rej/
>>>
> (with one exception, AnnotationTypeRuntimeAssumptionTest.java test was
> not included due to jdk8 API).

 Ah, It's the Class.getDeclaredAnnotation(Class) that's new in JDK8.
 Here's the changed test that only uses the JDK7 API so you can include
 this test too:

 http://cr.openjdk.java.net/~plevart/jdk7u/7122142/AnnotationTypeRuntimeAssumptionTest.java
>>>
>>> Thanks!
>>>
>>> http://cr.openjdk.java.net/~dmeetry/7122142.8005232.7185456.8022721/webrev.1/
>>>
>>> (just with the new test added).
>>>
>>> thanks,
>>> dmeetry
>
> All tests in test/java/lang/annotation passed.
>
> thanks,
> dmeetry

 Regards, Peter



-- 
Oracle 
Dalibor Topic | Principal Product Manager
Phone: +494089091214  | Mobile: +491737185961 

Oracle Java Platform Group

ORACLE Deutschland B.V. & Co. KG | Kühnehöfe 5 | 22761 Hamburg

ORACLE Deutschland B.V. & Co. KG
Hauptverwaltung: Riesstr. 25, D-80992 München
Registergericht: Amtsgericht München, HRA 95603
Geschäftsführer: Jürgen Kunz

Komplementärin: ORACLE Deutschland Verwaltung B.V.
Hertogswetering 163/167, 3543 AS Utrecht, Niederlande
Handelsregister der Handelskammer Midden-Niederlande, Nr. 30143697
Geschäftsführer: Alexander van der Ven, Astrid Kepper, Val Maher

Green Oracle  Oracle is committed to 
developing practices and products that help protect the environment

Re: [7u backport] RFR: 7122142: (ann) Race condition between isAnnotationPresent and getAnnotations

[dmeetry degrave]

Hi all,

I would like to ask someone with a reviewer status in jdk7u project to 
look at these changes.


thanks,
dmeetry

On 02/27/2014 05:44 PM, Joel Borggren-Franck wrote:

Hi,

I looked at webrev.1. Looks good.

cheers
/Joel

On 2014-02-25, dmeetry degrave wrote:

Thanks for looking at this, Peter!

On 02/24/2014 04:42 PM, Peter Levart wrote:

Hi Dmeetry,

On 02/22/2014 01:22 PM, dmeetry degrave wrote:

Hi all,

I would like to ask for a review of combined back port for
7u-dev/7u80. The main goal is to have a fix for 7122142 in jdk7, it
also integrates the changes from 8005232, 7185456, 8022721

https://bugs.openjdk.java.net/browse/JDK-7122142
https://bugs.openjdk.java.net/browse/JDK-8005232
https://bugs.openjdk.java.net/browse/JDK-7185456
https://bugs.openjdk.java.net/browse/JDK-8022721

Original jdk8 changes:

7122142: http://hg.openjdk.java.net/jdk8/tl/jdk/rev/e4ce6502eac0
8005232: http://hg.openjdk.java.net/jdk8/tl/jdk/rev/1109bfff4e92
7185456: http://hg.openjdk.java.net/jdk8/tl/jdk/rev/ae03282ba501
8022721: http://hg.openjdk.java.net/jdk8/tl/jdk/rev/2281a7f79738

back port:

http://cr.openjdk.java.net/~dmeetry/7122142.8005232.7185456.8022721/webrev.0/


Patches can't be applied cleanly, hence it was a manual back port,
though the final result is equivalent to applying the patches in
chronological order (8005232, 7185456, 7122142, 8022721) and applying
all the relevant rejected parts


It's good to see those patches being back-ported to 7u. By browsing the
webrev, I don't see any obvious difference between the original patches
and the backport.


there shouldn't be any!


Do you happen to remember in what part of code there
were rejects so that you had to manually apply the changes?


there were conflicts due to small difference between 7 and 8
(copyrights, white spaces, @SuppressWarnings, Class,...).

I copied all rejected parts and original patches here:

http://cr.openjdk.java.net/~dmeetry/7122142.8005232.7185456.8022721/webrev.1/rej/


(with one exception, AnnotationTypeRuntimeAssumptionTest.java test was
not included due to jdk8 API).


Ah, It's the Class.getDeclaredAnnotation(Class) that's new in JDK8.
Here's the changed test that only uses the JDK7 API so you can include
this test too:

http://cr.openjdk.java.net/~plevart/jdk7u/7122142/AnnotationTypeRuntimeAssumptionTest.java


Thanks!

http://cr.openjdk.java.net/~dmeetry/7122142.8005232.7185456.8022721/webrev.1/

(just with the new test added).

thanks,
dmeetry


All tests in test/java/lang/annotation passed.

thanks,
dmeetry


Regards, Peter

Re: [7u backport] RFR: 7122142: (ann) Race condition between isAnnotationPresent and getAnnotations

[Joel Borggren-Franck]

Hi,

I looked at webrev.1. Looks good.

cheers
/Joel

On 2014-02-25, dmeetry degrave wrote:
> Thanks for looking at this, Peter!
> 
> On 02/24/2014 04:42 PM, Peter Levart wrote:
> >Hi Dmeetry,
> >
> >On 02/22/2014 01:22 PM, dmeetry degrave wrote:
> >>Hi all,
> >>
> >>I would like to ask for a review of combined back port for
> >>7u-dev/7u80. The main goal is to have a fix for 7122142 in jdk7, it
> >>also integrates the changes from 8005232, 7185456, 8022721
> >>
> >>https://bugs.openjdk.java.net/browse/JDK-7122142
> >>https://bugs.openjdk.java.net/browse/JDK-8005232
> >>https://bugs.openjdk.java.net/browse/JDK-7185456
> >>https://bugs.openjdk.java.net/browse/JDK-8022721
> >>
> >>Original jdk8 changes:
> >>
> >>7122142: http://hg.openjdk.java.net/jdk8/tl/jdk/rev/e4ce6502eac0
> >>8005232: http://hg.openjdk.java.net/jdk8/tl/jdk/rev/1109bfff4e92
> >>7185456: http://hg.openjdk.java.net/jdk8/tl/jdk/rev/ae03282ba501
> >>8022721: http://hg.openjdk.java.net/jdk8/tl/jdk/rev/2281a7f79738
> >>
> >>back port:
> >>
> >>http://cr.openjdk.java.net/~dmeetry/7122142.8005232.7185456.8022721/webrev.0/
> >>
> >>
> >>Patches can't be applied cleanly, hence it was a manual back port,
> >>though the final result is equivalent to applying the patches in
> >>chronological order (8005232, 7185456, 7122142, 8022721) and applying
> >>all the relevant rejected parts
> >
> >It's good to see those patches being back-ported to 7u. By browsing the
> >webrev, I don't see any obvious difference between the original patches
> >and the backport.
> 
> there shouldn't be any!
> 
> >Do you happen to remember in what part of code there
> >were rejects so that you had to manually apply the changes?
> 
> there were conflicts due to small difference between 7 and 8
> (copyrights, white spaces, @SuppressWarnings, Class,...).
> 
> I copied all rejected parts and original patches here:
> 
> http://cr.openjdk.java.net/~dmeetry/7122142.8005232.7185456.8022721/webrev.1/rej/
> 
> >>(with one exception, AnnotationTypeRuntimeAssumptionTest.java test was
> >>not included due to jdk8 API).
> >
> >Ah, It's the Class.getDeclaredAnnotation(Class) that's new in JDK8.
> >Here's the changed test that only uses the JDK7 API so you can include
> >this test too:
> >
> >http://cr.openjdk.java.net/~plevart/jdk7u/7122142/AnnotationTypeRuntimeAssumptionTest.java
> 
> Thanks!
> 
> http://cr.openjdk.java.net/~dmeetry/7122142.8005232.7185456.8022721/webrev.1/
> 
> (just with the new test added).
> 
> thanks,
> dmeetry
> >>
> >>All tests in test/java/lang/annotation passed.
> >>
> >>thanks,
> >>dmeetry
> >
> >Regards, Peter
> >

Re: [7u backport] RFR: 7122142: (ann) Race condition between isAnnotationPresent and getAnnotations

[dmeetry degrave]

Thanks for looking at this, Peter!

On 02/24/2014 04:42 PM, Peter Levart wrote:

Hi Dmeetry,

On 02/22/2014 01:22 PM, dmeetry degrave wrote:

Hi all,

I would like to ask for a review of combined back port for
7u-dev/7u80. The main goal is to have a fix for 7122142 in jdk7, it
also integrates the changes from 8005232, 7185456, 8022721

https://bugs.openjdk.java.net/browse/JDK-7122142
https://bugs.openjdk.java.net/browse/JDK-8005232
https://bugs.openjdk.java.net/browse/JDK-7185456
https://bugs.openjdk.java.net/browse/JDK-8022721

Original jdk8 changes:

7122142: http://hg.openjdk.java.net/jdk8/tl/jdk/rev/e4ce6502eac0
8005232: http://hg.openjdk.java.net/jdk8/tl/jdk/rev/1109bfff4e92
7185456: http://hg.openjdk.java.net/jdk8/tl/jdk/rev/ae03282ba501
8022721: http://hg.openjdk.java.net/jdk8/tl/jdk/rev/2281a7f79738

back port:

http://cr.openjdk.java.net/~dmeetry/7122142.8005232.7185456.8022721/webrev.0/


Patches can't be applied cleanly, hence it was a manual back port,
though the final result is equivalent to applying the patches in
chronological order (8005232, 7185456, 7122142, 8022721) and applying
all the relevant rejected parts


It's good to see those patches being back-ported to 7u. By browsing the
webrev, I don't see any obvious difference between the original patches
and the backport.


there shouldn't be any!


Do you happen to remember in what part of code there
were rejects so that you had to manually apply the changes?


there were conflicts due to small difference between 7 and 8 
(copyrights, white spaces, @SuppressWarnings, Class,...).


I copied all rejected parts and original patches here:

http://cr.openjdk.java.net/~dmeetry/7122142.8005232.7185456.8022721/webrev.1/rej/


(with one exception, AnnotationTypeRuntimeAssumptionTest.java test was
not included due to jdk8 API).


Ah, It's the Class.getDeclaredAnnotation(Class) that's new in JDK8.
Here's the changed test that only uses the JDK7 API so you can include
this test too:

http://cr.openjdk.java.net/~plevart/jdk7u/7122142/AnnotationTypeRuntimeAssumptionTest.java


Thanks!

http://cr.openjdk.java.net/~dmeetry/7122142.8005232.7185456.8022721/webrev.1/

(just with the new test added).

thanks,
dmeetry


All tests in test/java/lang/annotation passed.

thanks,
dmeetry


Regards, Peter

Re: [7u backport] RFR: 7122142: (ann) Race condition between isAnnotationPresent and getAnnotations

[Peter Levart]

Hi Dmeetry,

On 02/22/2014 01:22 PM, dmeetry degrave wrote:

Hi all,

I would like to ask for a review of combined back port for 
7u-dev/7u80. The main goal is to have a fix for 7122142 in jdk7, it 
also integrates the changes from 8005232, 7185456, 8022721


https://bugs.openjdk.java.net/browse/JDK-7122142
https://bugs.openjdk.java.net/browse/JDK-8005232
https://bugs.openjdk.java.net/browse/JDK-7185456
https://bugs.openjdk.java.net/browse/JDK-8022721

Original jdk8 changes:

7122142: http://hg.openjdk.java.net/jdk8/tl/jdk/rev/e4ce6502eac0
8005232: http://hg.openjdk.java.net/jdk8/tl/jdk/rev/1109bfff4e92
7185456: http://hg.openjdk.java.net/jdk8/tl/jdk/rev/ae03282ba501
8022721: http://hg.openjdk.java.net/jdk8/tl/jdk/rev/2281a7f79738

back port:

http://cr.openjdk.java.net/~dmeetry/7122142.8005232.7185456.8022721/webrev.0/ 



Patches can't be applied cleanly, hence it was a manual back port, 
though the final result is equivalent to applying the patches in 
chronological order (8005232, 7185456, 7122142, 8022721) and applying 
all the relevant rejected parts


It's good to see those patches being back-ported to 7u. By browsing the 
webrev, I don't see any obvious difference between the original patches 
and the backport. Do you happen to remember in what part of code there 
were rejects so that you had to manually apply the changes?


(with one exception, AnnotationTypeRuntimeAssumptionTest.java test was 
not included due to jdk8 API).


Ah, It's the Class.getDeclaredAnnotation(Class) that's new in JDK8. 
Here's the changed test that only uses the JDK7 API so you can include 
this test too:


http://cr.openjdk.java.net/~plevart/jdk7u/7122142/AnnotationTypeRuntimeAssumptionTest.java



All tests in test/java/lang/annotation passed.

thanks,
dmeetry


Regards, Peter

[7u backport] RFR: 7122142: (ann) Race condition between isAnnotationPresent and getAnnotations

[dmeetry degrave]

Hi all,

I would like to ask for a review of combined back port for 7u-dev/7u80. 
The main goal is to have a fix for 7122142 in jdk7, it also integrates 
the changes from 8005232, 7185456, 8022721


https://bugs.openjdk.java.net/browse/JDK-7122142
https://bugs.openjdk.java.net/browse/JDK-8005232
https://bugs.openjdk.java.net/browse/JDK-7185456
https://bugs.openjdk.java.net/browse/JDK-8022721

Original jdk8 changes:

7122142: http://hg.openjdk.java.net/jdk8/tl/jdk/rev/e4ce6502eac0
8005232: http://hg.openjdk.java.net/jdk8/tl/jdk/rev/1109bfff4e92
7185456: http://hg.openjdk.java.net/jdk8/tl/jdk/rev/ae03282ba501
8022721: http://hg.openjdk.java.net/jdk8/tl/jdk/rev/2281a7f79738

back port:

http://cr.openjdk.java.net/~dmeetry/7122142.8005232.7185456.8022721/webrev.0/

Patches can't be applied cleanly, hence it was a manual back port, 
though the final result is equivalent to applying the patches in 
chronological order (8005232, 7185456, 7122142, 8022721) and applying 
all the relevant rejected parts (with one exception, 
AnnotationTypeRuntimeAssumptionTest.java test was not included due to 
jdk8 API).


All tests in test/java/lang/annotation passed.

thanks,
dmeetry