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.
