7122142 : (ann) Race condition between isAnnotationPresent and getAnnotations

[Peter Levart]

Hi,

Since the bulk of changes to annotations and reflection have stabilized, 
I'm bringing up a re-based batch that I have proposed some months ago:

http://mail.openjdk.java.net/pipermail/core-libs-dev/2013-February/014203.html

The patch fixes a deadlock situation described in:

     http://bugs.sun.com/bugdatabase/view_bug.do?bug_id=7122142

...and also a logical inconsistency when parsing mutually-recursive 
runtime annotations combined with change of @Retention and separate 
compilation (described in above thread).

Here's the webrev for the patch:

http://cr.openjdk.java.net/~plevart/jdk8-tl/AnnotationType/webrev.01/

The deadlock occurs because two object monitors are tried to be acquired 
by two threads in different order. One object monitor is represented by 
static synchronized AnnotationType.getInstance(Class) method, the other 
is synchronized Class.initAnnotationsIfNecessary() instance method. 
There are various scenarios where these two methods can be entered by 
different threads in opposite order and for same j.l.Class instance.

AnnotationType.getInstance(Class) is synchronized, because it guards 
access to j.l.Class.annotationType field while AnnotationType instance 
for particular Class is being constructed. AnnotationType instance is 
published via Class.annotationType field in the middle of AnnotationType 
constructor so that annotation's meta-annotations can be obtained after 
that point as the final act of AnnotationType construction. While 
meta-annotations are being obtained, other meta-annotations (besides 
standard @Retention and @Inhereted) can be parsed for the 1st time, 
which triggers AnnotationType.getInstance(Class) call for those 
meta-annotation classes. If such meta-annotation classes are mutually 
recursive with the annotation class that AnnotationType is being 
constructed for, AnnotationType.getInstance(Class) can be re-entered for 
the same annotation class. To prevent infinite recursion, 
half-constructed AnnotationType instance is published in the middle of 
the AnnotationType constructor. Synchronized AnnotationType.getInstance 
prevents other threads to interfere while AnnotationType is being 
constructed, but allows re-entrancy from the same thread.

The presented patch solves this situation by removing the synchronized 
keyword from AnnotationType.getInstance(Class) static method, Making 
this method contention-free as a benefit. All other changes are a result 
of that removal:
- AnnotationType class is made immutable (all fields final)
- The instance is published only after the constructor for 
AnnotationType returns (in AnnotationType.getInstance() method). Because 
caching is idempotent and AnnotationType object is immutable, no 
synchronization is attempted and not even Class.annotationType filed is 
made volatile.
- To prevent infinite recursion while obtaining meta-annotations, the 
process of obtaining them was redesigned. Instead of calling 
getAnnotation(Retention.class) or isAnnotationPresent(Inherited.class) 
which triggers parsing of other meta-annotations too, special method was 
added to AnnotationParser which is used solely to parse select 
meta-annotations in the AnnotationType constructor and which does not 
need to evaluate AnnotationType.getInstance() method, thus making this 
parsing method recursion-free.

The benefit of not recursing while obtaining meta-annotations is also 
more correct logic when obtaining mutualy-recursive annotations for 
which one of them was changed (@Retention: RUNTIME -> CLASS) and 
separately compiled.

This patch is one angle of attack for bug 7122142. The other is, off 
course, removing the other synchronized keyword (on 
Class.initAnnotationsIfNecessary()). I'm planning to do that too, but in 
a more straight-forward manner.

Regards, Peter