Reviewed a bit more this part and think we can just drop the 2 structure
state to only use the concurrent map we already have, the computeIfAbsent
overhead will be negligible compared to the simple contains and it will
solve by construction the locking issue:
diff --git
a/webbeans-impl/src/main/java/org/apache/webbeans/annotation/AnnotationManager.java
b/webbeans-impl/src/main/java/org/apache/webbeans/annotation/AnnotationManager.java
index 24033f4ec..b5442c446 100644
---
a/webbeans-impl/src/main/java/org/apache/webbeans/annotation/AnnotationManager.java
+++
b/webbeans-impl/src/main/java/org/apache/webbeans/annotation/AnnotationManager.java
@@ -62,7 +62,7 @@ import java.util.Map;
import java.util.Optional;
import java.util.Set;
import java.util.concurrent.ConcurrentHashMap;
-import java.util.concurrent.CopyOnWriteArraySet;
+import java.util.concurrent.ConcurrentMap;
/**
* Manages annotation usage by classes in this application.
@@ -74,8 +74,7 @@ public final class AnnotationManager
private Map<Class<? extends Annotation>, Boolean>
checkedStereotypeAnnotations =
new ConcurrentHashMap<>();
- private CopyOnWriteArraySet<Class<?>> repeatableMethodCheckedTypes =
new CopyOnWriteArraySet<>();
- private Map<Class<?>, Optional<Method>> repeatableMethodCache = new
ConcurrentHashMap<>();
+ private ConcurrentMap<Class<?>, Optional<Method>>
repeatableMethodCache = new ConcurrentHashMap<>();
private final BeanManagerImpl beanManagerImpl;
private final WebBeansContext webBeansContext;
@@ -938,23 +937,12 @@ public final class AnnotationManager
public void clearCaches()
{
- repeatableMethodCheckedTypes.clear();
repeatableMethodCache.clear();
}
public Optional<Method> getRepeatableMethod(Class<?> type)
{
- if (repeatableMethodCheckedTypes.contains(type))
- {
- return repeatableMethodCache.get(type);
- }
-
- Optional<Method> method =
Optional.ofNullable(resolveRepeatableMethod(type));
-
- repeatableMethodCheckedTypes.add(type);
- repeatableMethodCache.put(type, method); // don't put null here!
-
- return method;
+ return repeatableMethodCache.computeIfAbsent(type, it ->
Optional.ofNullable(resolveRepeatableMethod(it)));
}
protected Method resolveRepeatableMethod(Class<?> type)
still needs the stack to validate the hypothesis but even if it does not
fix the original issue it looks better in terms of impl, wdyt?
Romain Manni-Bucau
@rmannibucau <https://twitter.com/rmannibucau> | Blog
<https://rmannibucau.metawerx.net/> | Old Blog
<http://rmannibucau.wordpress.com> | Github <https://github.com/rmannibucau> |
LinkedIn <https://www.linkedin.com/in/rmannibucau> | Book
<https://www.packtpub.com/application-development/java-ee-8-high-performance>
Le mar. 28 juil. 2020 à 21:39, Romain Manni-Bucau <[email protected]> a
écrit :
> Hi
>
> It is a startup method normally so synchro should be useless until you
> abuse of createAnnotatedType at runtime - which can be another library bug
> ;).
>
> Would be great to have more of the stacktrace to check it - which means we
> would lock only this specific runtime api and not startup ones (we have
> that state and there are different methods calling it).
>
> Hope it makes sense
>
> Le mar. 28 juil. 2020 à 11:20, Vicente Rossello <[email protected]>
> a écrit :
>
>> Hi,
>>
>> In our CI server I can see an occasional NPE:
>>
>> Caused by: java.lang.NullPointerException: Cannot invoke
>> "java.util.Optional.isPresent()" because "repeatableMethod" is null
>> at
>> org.apache.webbeans.portable.AbstractAnnotated.buildRepeatableAnnotations(AbstractAnnotated.java:107)
>> at
>> org.apache.webbeans.portable.AbstractAnnotated.setAnnotations(AbstractAnnotated.java:166)
>> at
>> org.apache.webbeans.portable.AnnotatedTypeImpl.<init>(AnnotatedTypeImpl.java:76)
>> at
>> org.apache.webbeans.portable.AnnotatedElementFactory.newAnnotatedType(AnnotatedElementFactory.java:183)
>>
>>
>> (It's a test that spans some threads and persists some concurrent data).
>>
>>
>> It happens once a day at most, and I think it happens lately since I
>> upgraded to 2.0.16 or 17 (I have no idea why because this hasn't changed in
>> a while, I can't confirm this)
>>
>>
>> I've been unable to reproduce it locally neither.
>>
>>
>> The thing is that looking at AnnotationManager.java:
>>
>>
>> public Optional<Method> getRepeatableMethod(Class<?> type)
>> {
>> if (repeatableMethodCheckedTypes.contains(type))
>> {
>> return repeatableMethodCache.get(type);
>> }
>>
>> Optional<Method> method =
>> Optional.ofNullable(resolveRepeatableMethod(type));
>>
>> repeatableMethodCheckedTypes.add(type);
>> repeatableMethodCache.put(type, method); // don't put null here!
>>
>> return method;
>> }
>>
>>
>> The repeatableMethodCheckedTypes can have the value and the
>> repeatableMethodCache empty at the same time, which I guess is what's
>> happening here.
>>
>>
>> A solution can be to synchronize the add and put, or the
>> repeatableMethodCheckedTypes can be removed (it's not used anywhere else, I
>> guess it's there for performance reasons?)
>>
>> WDYT?
>>
>>
