7122142 : (ann) Race condition between isAnnotationPresent and getAnnotations

[Peter Levart]

Hi Alan, Joel,

Thanks to both for taking time to review the patch.

Here's the 3rd revision:

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

Answers inline...

On 07/03/2013 04:51 PM, Alan Bateman wrote:
> Sorry for the delay getting back to you on this, I've been busy with 
> other things.

Me too.

>
> 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).

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.

>
> 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.

I tailored new method to be consistent with the alignment of the other 
public method. There are 4 different multi-line-parameter alignments 
used in AnnotationParser though. I also renamed new method to 
"parseSelectAnnotations" and made it package-private, since it is only 
used from the AnnotationType.

>
> 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. 

Done.

> 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.

Right, There can be a substantial delay from Thread.start() to thread 
actually executing it's run() method. So I added another synchronization 
latch that makes main thread park  and wait until both new threads 
start-up. Both new threads then spin-wait until main thread wakes up and 
resets the AtomicInteger value. Experimenting on my Intel i7 box shows 
that one new thread spins about 80x and the other about 3000x before 
continuing. This spin-waiting is necessary so that both threads continue 
coherently and have a better chance to provoke deadlock.

>
> 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.

Corrected.

On 07/03/2013 07:09 PM, Joel Borggrén-Franck wrote:
> 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.

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?

> 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.

I missed that, yes. I changed the patch so that no changes are needed now.

> 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.

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.

Regards, Peter

> Given that and that you fix Alan's comments I think this is good to go.
>
> Thanks again!
>
> cheers
> /Joel
>