On 24/06/2013 19:23, Peter Levart wrote:

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.

Sorry for the delay getting back to you on this, I've been busy with other things.

I'm not an expert on the annotation code but the updated approach to break the recursion looks good good to me (and a clever approach). Joel has been on vacation but he told me that he plans to look at this too (I'm happy to sponsor it in the mean-time as I think the approach is good and we should get this fix in).

There's another usage of AnnotationParser.parseAnnotation in TypeAnnotationParser that will need to be updated (I only noticed it when I grabbed your patch to try it).

A minor comment is that the alignment of the parameter declarations when they go over multiple lines should probably be fixed up to be consistent with the other usages.

Thanks for adding tests. One comment on AnnotationTypeDeadlockTest is that the join(500) might not be sufficient on a very busy system (say when running tests concurrently) or on a low-end embedded system. So I think it would be good to bump this up x10. An alternative would be to not set the daemon status and just let the test timeout if there is a deadlock. The spin-waits can consume cycles when running tests concurrently but I don't expect it's an issue here.

A typo in the @summary of AnnotationTypeRuntimeAssumptionTest ("si" -> "is"). I guess I'd go for slightly shorter lines to make future side-by-side reviews easier.

Otherwise, this looks good to me.

-Alan.









Reply via email to