RFR 7122142 : (ann) Race condition between isAnnotationPresent and getAnnotations

Peter Levart peter.levart at gmail.com
Mon Jul 15 11:41:22 UTC 2013

Hi David,

I think now that recursion is broken by special-purpose parsing 
code-path and, depending on how we solve the other contention point in 
j.l.Class.initAnnotationsIfNecessary(), there might be no danger of 
dead-lock even if we used double-checked locking idiom in 
AnnotationType.getInstance(). We could re-introduce a singleton lock, 
which would mean contention when a lot of distinct annotation types need 
to be constructed at the same time by multiple threads. Or the 
synchronization could be performed on the 'annotationClass' instance, 
which is prone to deadlock interaction with user code that synchronizes 
on the j.l.Class objects while obtaining annotations.

So you're right: there would either have to be some private lock object 
referenced from j.l.Class or an indirection via some kind of Future 
which memoizes the result for further invocations and which (the Future) 
has to be installed atomically into the j.l.Class field.

But as parsing 2 small annotations and skipping the rest out from a 
byte[] of raw annotations is a relatively fast operation (I measured 
about 6...8 μs on my i7 PC depending on whether there were lots of other 
meta-annotations present on the annotation or not), I think there will 
be no problem in wasted CPU cycles even If we keep optimistic 
construction. But we'll see.

Regards, Peter

On 07/15/2013 07:45 AM, David Holmes wrote:
> Joel, Peter,
> I've looked at the synchronization changes and the use of CAS and it 
> seems sound.
> The only performance-related concern is the wasted effort when 
> multiple threads each call "new AnnotationType(annotationClass)". But 
> if that turns out to be an issue we can address it (using memoization 
> pattern that installs a Future for the AnnotationType).
> Thanks,
> David
>>> *From: *Peter Levart <peter.levart at gmail.com
>>> <mailto:peter.levart at gmail.com>>
>>> *Subject: **RFR 7122142 : (ann) Race condition between
>>> isAnnotationPresent and getAnnotations*
>>> *Date: *8 juli 2013 22:54:12 CEST
>>> *To: *Joel Borggrén-Franck <joel.franck at oracle.com
>>> <mailto:joel.franck at oracle.com>>
>>> *Cc: *Alan Bateman <Alan.Bateman at oracle.com
>>> <mailto:Alan.Bateman at oracle.com>>, "core-libs-dev at openjdk.java.net
>>> <mailto:core-libs-dev at openjdk.java.net> core-libs-dev"
>>> <core-libs-dev at openjdk.java.net 
>>> <mailto:core-libs-dev at openjdk.java.net>>
>>> Helo,
>>> I need a Reviewer for the following patch:
>>> http://cr.openjdk.java.net/~plevart/jdk8-tl/AnnotationType/webrev.05/
>>> This fixes deadlock situation described in bug:
>>> http://bugs.sun.com/bugdatabase/view_bug.do?bug_id=7122142
>>> The in-depth evaluation of the patch is described in the introductory
>>> message of this thread:
>>> http://mail.openjdk.java.net/pipermail/core-libs-dev/2013-June/018226.html 
>>> The above is the 5th revision of the patch that also makes sure a
>>> single AnnotationType instance is ever returned for the same Class
>>> object even when multiple threads concurrently request one.
>>> Regards, Peter
>>> On 07/05/2013 04:04 PM, Joel Borggrén-Franck wrote:
>>>> Hi Peter,
>>>> Thanks for the quick update!
>>>> While I have looked over the changes to j.l.Class and the cas in 
>>>> AnnotationType I don't think I'm qualified to review that. (FWIW it 
>>>> looked fine to me but my jmm isn't swapped in at the moment so I 
>>>> won't pretend to know the interactions between volatile and Unsafe 
>>>> cas). Thinking out loud I suppose we can assume a stable offset of 
>>>> fields in Class, and I realize that part has been under review before.
>>>> The rest of AnnotationType, AnnotationParser and the tests look 
>>>> fine though. I also ran the tests before and after the change and 
>>>> results were as expected.
>>>> Since I'm not a Reviewer kind of reviewer you need to get one those 
>>>> to sign off but after that I think you are good to go. I can 
>>>> sponsor this as well if Alan is busy.
>>>> cheers
>>>> /Joel
>>>> On 5 jul 2013, at 11:12, Peter Levart<peter.levart at gmail.com> wrote:
>>>>> Hi Again,
>>>>> Sorry, the 4th revision I posted is not rebased to the current tip 
>>>>> of jdk8-tl so it contained some diffs that reverted some things.
>>>>> Here's the correct patch:
>>>>> http://cr.openjdk.java.net/~plevart/jdk8-tl/AnnotationType/webrev.05/
>>>>> Regards, Peter
>>>>> On 07/05/2013 10:32 AM, Peter Levart wrote:
>>>>>> Hi Joel,
>>>>>> Here's the 4th revision of the patch:
>>>>>> http://cr.openjdk.java.net/~plevart/jdk8-tl/AnnotationType/webrev.04/ 
>>>>>> This one introduces CAS-ing of the AnnotationType instance into 
>>>>>> the Class.annotationType field so that there's only a single 
>>>>>> instance ever returned for a single Class. I also introduced new 
>>>>>> private static Class.Atomic nested class that is used to lazily 
>>>>>> initialize Unsafe machinery and to provide a safe wrapper for 
>>>>>> internal j.l.Class use. Previously this was part of 
>>>>>> ReflectionData nested class because it was only used for it's 
>>>>>> purpose. In anticipation to have a need for more atomic 
>>>>>> operations in the future, I moved this code to a separate class. 
>>>>>> ReflectionData is now just a record.
>>>>>> Regards, Peter

More information about the core-libs-dev mailing list