7122142 : (ann) Race condition between isAnnotationPresent and getAnnotations

[Joel Borggrén-Franck]

Hi Peter,

On 4 jul 2013, at 16:40, Peter Levart <peter.levart at gmail.com> wrote:

> Answers inline...
>> 
>> 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). 
> 
> I rather restored the package-private API so that TypeAnnotationParser doesn't have to be changed. It also makes for less changes in internal AnnotationParser code.
> 

I agree.

> 
> I'm interested in your approach. How do you handle construction of AnnotationType instances for other annotations apart from @Retention and @Inherited? What if there are two mutually recursive annotations like:
> 
> 
>     @Retention(RUNTIME)
>     @AnnB
>     public @interface AnnA {
>     }
> 
>     @Retention(RUNTIME)
>     @AnnA
>     public @interface AnnB {
>     }
> 
> How do you construct the AnnotationType instance for any one of the above annotations and break recursion?
> 

To be honest, I didn't get that far before it became clear to me that the approach wouldn't be preferable to this one. I needed to break that recursion somehow and that turned out to become something like parseSelectedAnnotations(...) anyway so I just put the idea aside.

>> 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.
> 
> Well, one thing is that AnnotationType class is now effectively final, meaning that all it's state is referenced using final fields. If a reference to an instance of such class is obtained via data-race, all it's state is observed consistent by the thread that obtained the reference. The other thing is racy caching. If two or more threads are concurrently requesting AnnotationType for the same Class, many of them might construct and use it's own instance and the one published "latest" will finally be the one being cached. Since all such AnnotationType instances are constructed from the same input data, they are equivalent and it doesn't matter which one is used by which thread.
> 

Actually they aren't, something have been bugging me and I finally figured it out today. The problem is with default valued elements. Previously all threads have been seeing the same instance of default values but now that will only be guaranteed for Classes, Strings and Integers inside the value cache. While I don't think it is in the spec that annotation default value instances should be == for all threads I don't think it is a race we should introduce. Given this I think you should use some approach to produce a winner that every thread will use (like in the other annotations patch). My gut feeling is that CASing in a result will be better under contention that the current lock solution (while also correct and deadlock free) for a net win.

> There is a caveat though. What if class is redefined? That's a separate issue and will have to be resolved in a separate patch. I'm planing to prepare a patch after this one gets through. The patch will remove contention from caching of annotations and may also take care of AnnotationType caching at the same time.

I can't imagine this solution being more broken that the current situation :) or?

cheers
/Joel