Hi Peter, As Alan said, a big thanks for looking into this.
I have been toying with a slightly different approach to breaking the infinite recursion by pre-construct AnnotationType instances for Retention and Inherited. While cleaner in some places others became uglier. I'm fine with this approach. As Alan said, you need to update the patch with a modification in TypeAnnotationParser. After doing that all java/lang tests in the jdk/test directory passes. Also, can you please explain to me why the update race is safe. I have done the/some research myself but I would like to know which angles you have covered. Given that and that you fix Alan's comments I think this is good to go. Thanks again! cheers /Joel On Jun 24, 2013, at 8:23 PM, Peter Levart <peter.lev...@gmail.com> wrote: > On 06/19/2013 08:54 PM, Alan Bateman wrote: >> Thank you for coming back to this. >> >> I've looked over the webrev and the approach looks good to me. Joel might >> want to look at this too. Do you think you could include a test (as we try >> to include a test with all fixes if we can)? >> >> It would be good to remove the synchronizaiton on initAnnotationsIfNecessary >> too, but one step as time (and smaller changes are always easier to discuss). >> >> -Alan > > Hi Alan, > > I have prepared the 2nd revision of the patch: > > http://cr.openjdk.java.net/~plevart/jdk8-tl/AnnotationType/webrev.02/ > > There is a little change in AnnotationParser's alternative parsing method. > This time the parser does not assume anything about annotations being parsed > (previously it assumed that they had RUNTIME retention). The only difference > from standard parsing is that only the select annotation types are parsed and > the rest are quickly skipped. Infinite recursion is broken by the special > cased evaluation in AnnotationType constructor: > > 129 // Initialize retention, & inherited fields. Special treatment > 130 // of the corresponding annotation types breaks infinite > recursion. > 131 if (annotationClass != Retention.class && > 132 annotationClass != Inherited.class) { > 133 JavaLangAccess jla = > sun.misc.SharedSecrets.getJavaLangAccess(); > 134 Map<Class<? extends Annotation>, Annotation> metaAnnotations = > 135 AnnotationParser.parseAnnotations( > 136 jla.getRawClassAnnotations(annotationClass), > 137 jla.getConstantPool(annotationClass), > 138 annotationClass, > 139 Retention.class, Inherited.class > 140 ); > 141 Retention ret = (Retention) > metaAnnotations.get(Retention.class); > 142 retention = (ret == null ? RetentionPolicy.CLASS : > ret.value()); > 143 inherited = metaAnnotations.containsKey(Inherited.class); > 144 } > 145 else { > 146 retention = RetentionPolicy.RUNTIME; > 147 inherited = false; > 148 } > > > This is enough to break recursion. The RUNTIME and !inherited assumptions for > @Retention and @Inherited meta-annotations are therefore localized in this > code only. > > I have also added two tests. The one for detecting deadlock situation and the > other for consistent parsing of mutually recursive annotations in presence of > separate compilation. > > > > Regards, Peter >
