Your change is good, I did the same thing without the computeIfAbsent, I prefer yours, simpler
On Wed, Jul 29, 2020 at 12:29 PM Romain Manni-Bucau <[email protected]> wrote: > Oki, it is > https://github.com/apache/tomee/blob/8f6bd834a37a8745331c6580c458813e713db92c/container/openejb-core/src/main/java/org/apache/openejb/cdi/transactional/InterceptorBase.java#L92 > which > is really a runtime call and I think the original author wanted to keep it > lazy since it is only about the error case (;)). > Do you have another fix than the concurrent map one or should I push it as > this - also happy to go through a PR if you have something handy? > > 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 mer. 29 juil. 2020 à 11:46, Vicente Rossello <[email protected]> > a écrit : > >> Hi, I did this change (similar) in our build yesterday and everything >> works fine. >> >> The whole stacktrace is: >> >> java.util.concurrent.ExecutionException: java.lang.RuntimeException: >> java.lang.NullPointerException: Cannot invoke >> "java.util.Optional.isPresent()" because "repeatableMethod" is null >> at java.base/java.util.concurrent.FutureTask.report(FutureTask.java:122) >> at java.base/java.util.concurrent.FutureTask.get(FutureTask.java:205) >> at org.apache.openejb.threads.future.CUFuture.get(CUFuture.java:60) >> at >> com.infratr2.interceptor.async.FutureDelegator.get(FutureDelegator.java:23) >> at >> com.tr2.test.TestDataRepository.saveConcurrentDatasheets(TestDataRepository.java:232) >> at >> com.tr2.test.TestDataRepository$$OwbInterceptProxy0.saveConcurrentDatasheets(com/tr2/test/TestDataRepository.java) >> at >> com.tr2.test.TestDataRepository$$OwbNormalScopeProxy0.saveConcurrentDatasheets(com/tr2/test/TestDataRepository.java) >> at >> com.tr2.web.business.HotelDataSheetServiceIT.save_concurrent_hoteldatasheets(HotelDataSheetServiceIT.java:40) >> at >> java.base/jdk.internal.reflect.NativeMethodAccessorImpl.invoke0(Native >> Method) >> at >> java.base/jdk.internal.reflect.NativeMethodAccessorImpl.invoke(NativeMethodAccessorImpl.java:62) >> at >> java.base/jdk.internal.reflect.DelegatingMethodAccessorImpl.invoke(DelegatingMethodAccessorImpl.java:43) >> at java.base/java.lang.reflect.Method.invoke(Method.java:564) >> at >> org.junit.platform.commons.util.ReflectionUtils.invokeMethod(ReflectionUtils.java:686) >> at >> org.junit.jupiter.engine.execution.MethodInvocation.proceed(MethodInvocation.java:60) >> at >> org.junit.jupiter.engine.execution.InvocationInterceptorChain$ValidatingInvocation.proceed(InvocationInterceptorChain.java:131) >> at >> org.junit.jupiter.engine.extension.TimeoutExtension.intercept(TimeoutExtension.java:149) >> at >> org.junit.jupiter.engine.extension.TimeoutExtension.interceptTestableMethod(TimeoutExtension.java:140) >> at >> org.junit.jupiter.engine.extension.TimeoutExtension.interceptTestMethod(TimeoutExtension.java:84) >> at >> org.junit.jupiter.engine.execution.ExecutableInvoker$ReflectiveInterceptorCall.lambda$ofVoidMethod$0(ExecutableInvoker.java:115) >> at >> org.junit.jupiter.engine.execution.ExecutableInvoker.lambda$invoke$0(ExecutableInvoker.java:105) >> at >> org.junit.jupiter.engine.execution.InvocationInterceptorChain$InterceptedInvocation.proceed(InvocationInterceptorChain.java:106) >> at >> org.junit.jupiter.engine.execution.InvocationInterceptorChain.proceed(InvocationInterceptorChain.java:64) >> at >> org.junit.jupiter.engine.execution.InvocationInterceptorChain.chainAndInvoke(InvocationInterceptorChain.java:45) >> at >> org.junit.jupiter.engine.execution.InvocationInterceptorChain.invoke(InvocationInterceptorChain.java:37) >> at >> org.junit.jupiter.engine.execution.ExecutableInvoker.invoke(ExecutableInvoker.java:104) >> at >> org.junit.jupiter.engine.execution.ExecutableInvoker.invoke(ExecutableInvoker.java:98) >> at >> org.junit.jupiter.engine.descriptor.TestMethodTestDescriptor.lambda$invokeTestMethod$6(TestMethodTestDescriptor.java:212) >> at >> org.junit.platform.engine.support.hierarchical.ThrowableCollector.execute(ThrowableCollector.java:73) >> at >> org.junit.jupiter.engine.descriptor.TestMethodTestDescriptor.invokeTestMethod(TestMethodTestDescriptor.java:208) >> at >> org.junit.jupiter.engine.descriptor.TestMethodTestDescriptor.execute(TestMethodTestDescriptor.java:137) >> at >> org.junit.jupiter.engine.descriptor.TestMethodTestDescriptor.execute(TestMethodTestDescriptor.java:71) >> at >> org.junit.platform.engine.support.hierarchical.NodeTestTask.lambda$executeRecursively$5(NodeTestTask.java:135) >> at >> org.junit.platform.engine.support.hierarchical.ThrowableCollector.execute(ThrowableCollector.java:73) >> at >> org.junit.platform.engine.support.hierarchical.NodeTestTask.lambda$executeRecursively$7(NodeTestTask.java:125) >> at >> org.junit.platform.engine.support.hierarchical.Node.around(Node.java:135) >> at >> org.junit.platform.engine.support.hierarchical.NodeTestTask.lambda$executeRecursively$8(NodeTestTask.java:123) >> at >> org.junit.platform.engine.support.hierarchical.ThrowableCollector.execute(ThrowableCollector.java:73) >> at >> org.junit.platform.engine.support.hierarchical.NodeTestTask.executeRecursively(NodeTestTask.java:122) >> at >> org.junit.platform.engine.support.hierarchical.NodeTestTask.execute(NodeTestTask.java:80) >> at >> org.junit.platform.engine.support.hierarchical.ForkJoinPoolHierarchicalTestExecutorService$ExclusiveTask.compute(ForkJoinPoolHierarchicalTestExecutorService.java:171) >> at >> org.junit.platform.engine.support.hierarchical.ForkJoinPoolHierarchicalTestExecutorService.invokeAll(ForkJoinPoolHierarchicalTestExecutorService.java:115) >> at >> org.junit.platform.engine.support.hierarchical.NodeTestTask.lambda$executeRecursively$5(NodeTestTask.java:139) >> at >> org.junit.platform.engine.support.hierarchical.ThrowableCollector.execute(ThrowableCollector.java:73) >> at >> org.junit.platform.engine.support.hierarchical.NodeTestTask.lambda$executeRecursively$7(NodeTestTask.java:125) >> at >> org.junit.platform.engine.support.hierarchical.Node.around(Node.java:135) >> at >> org.junit.platform.engine.support.hierarchical.NodeTestTask.lambda$executeRecursively$8(NodeTestTask.java:123) >> at >> org.junit.platform.engine.support.hierarchical.ThrowableCollector.execute(ThrowableCollector.java:73) >> at >> org.junit.platform.engine.support.hierarchical.NodeTestTask.executeRecursively(NodeTestTask.java:122) >> at >> org.junit.platform.engine.support.hierarchical.NodeTestTask.execute(NodeTestTask.java:80) >> at >> org.junit.platform.engine.support.hierarchical.ForkJoinPoolHierarchicalTestExecutorService$ExclusiveTask.compute(ForkJoinPoolHierarchicalTestExecutorService.java:171) >> at >> org.junit.platform.engine.support.hierarchical.ForkJoinPoolHierarchicalTestExecutorService.executeNonConcurrentTasks(ForkJoinPoolHierarchicalTestExecutorService.java:141) >> at >> org.junit.platform.engine.support.hierarchical.ForkJoinPoolHierarchicalTestExecutorService.invokeAll(ForkJoinPoolHierarchicalTestExecutorService.java:121) >> at >> org.junit.platform.engine.support.hierarchical.NodeTestTask.lambda$executeRecursively$5(NodeTestTask.java:139) >> at >> org.junit.platform.engine.support.hierarchical.ThrowableCollector.execute(ThrowableCollector.java:73) >> at >> org.junit.platform.engine.support.hierarchical.NodeTestTask.lambda$executeRecursively$7(NodeTestTask.java:125) >> at >> org.junit.platform.engine.support.hierarchical.Node.around(Node.java:135) >> at >> org.junit.platform.engine.support.hierarchical.NodeTestTask.lambda$executeRecursively$8(NodeTestTask.java:123) >> at >> org.junit.platform.engine.support.hierarchical.ThrowableCollector.execute(ThrowableCollector.java:73) >> at >> org.junit.platform.engine.support.hierarchical.NodeTestTask.executeRecursively(NodeTestTask.java:122) >> at >> org.junit.platform.engine.support.hierarchical.NodeTestTask.execute(NodeTestTask.java:80) >> at >> org.junit.platform.engine.support.hierarchical.ForkJoinPoolHierarchicalTestExecutorService$ExclusiveTask.compute(ForkJoinPoolHierarchicalTestExecutorService.java:171) >> at >> java.base/java.util.concurrent.RecursiveAction.exec(RecursiveAction.java:189) >> at >> java.base/java.util.concurrent.ForkJoinTask.doExec(ForkJoinTask.java:290) >> at >> java.base/java.util.concurrent.ForkJoinPool$WorkQueue.topLevelExec(ForkJoinPool.java:1016) >> at >> java.base/java.util.concurrent.ForkJoinPool.scan(ForkJoinPool.java:1665) >> at >> java.base/java.util.concurrent.ForkJoinPool.runWorker(ForkJoinPool.java:1598) >> at >> java.base/java.util.concurrent.ForkJoinWorkerThread.run(ForkJoinWorkerThread.java:183) >> Caused by: java.lang.RuntimeException: java.lang.NullPointerException: >> Cannot invoke "java.util.Optional.isPresent()" because "repeatableMethod" is >> null >> at >> org.apache.webbeans.portable.AnnotatedElementFactory.newAnnotatedType(AnnotatedElementFactory.java:204) >> at >> org.apache.webbeans.container.BeanManagerImpl.createAnnotatedType(BeanManagerImpl.java:585) >> at >> org.apache.webbeans.container.InjectableBeanManager.createAnnotatedType(InjectableBeanManager.java:91) >> at >> org.apache.openejb.cdi.transactional.InterceptorBase.intercept(InterceptorBase.java:92) >> at >> org.apache.openejb.cdi.transactional.RequiredInterceptor.intercept(RequiredInterceptor.java:35) >> at jdk.internal.reflect.GeneratedMethodAccessor125.invoke(Unknown >> Source) >> at >> java.base/jdk.internal.reflect.DelegatingMethodAccessorImpl.invoke(DelegatingMethodAccessorImpl.java:43) >> at java.base/java.lang.reflect.Method.invoke(Method.java:564) >> at >> org.apache.webbeans.component.InterceptorBean.intercept(InterceptorBean.java:136) >> at >> org.apache.webbeans.intercept.InterceptorInvocationContext.proceed(InterceptorInvocationContext.java:65) >> at >> org.apache.webbeans.intercept.DefaultInterceptorHandler.invoke(DefaultInterceptorHandler.java:139) >> at >> com.tr2.engine.business.HotelDataSheetRepository$$OwbInterceptProxy0.save(com/tr2/engine/business/HotelDataSheetRepository.java) >> at >> com.tr2.engine.business.HotelDataSheetRepository$$OwbNormalScopeProxy0.save(com/tr2/engine/business/HotelDataSheetRepository.java) >> at >> com.tr2.engine.HotelDataSheetService.saveDataSheet(HotelDataSheetService.java:44) >> at >> java.base/jdk.internal.reflect.NativeMethodAccessorImpl.invoke0(Native >> Method) >> at >> java.base/jdk.internal.reflect.NativeMethodAccessorImpl.invoke(NativeMethodAccessorImpl.java:62) >> at >> java.base/jdk.internal.reflect.DelegatingMethodAccessorImpl.invoke(DelegatingMethodAccessorImpl.java:43) >> at java.base/java.lang.reflect.Method.invoke(Method.java:564) >> at >> org.apache.webbeans.intercept.AbstractInvocationContext.directProceed(AbstractInvocationContext.java:113) >> at >> org.apache.webbeans.intercept.AbstractInvocationContext.proceed(AbstractInvocationContext.java:106) >> at >> org.apache.webbeans.intercept.InterceptorInvocationContext.proceed(InterceptorInvocationContext.java:78) >> at >> org.apache.openejb.cdi.transactional.InterceptorBase.intercept(InterceptorBase.java:67) >> at >> org.apache.openejb.cdi.transactional.RequiredInterceptor.intercept(RequiredInterceptor.java:35) >> at jdk.internal.reflect.GeneratedMethodAccessor125.invoke(Unknown >> Source) >> at >> java.base/jdk.internal.reflect.DelegatingMethodAccessorImpl.invoke(DelegatingMethodAccessorImpl.java:43) >> at java.base/java.lang.reflect.Method.invoke(Method.java:564) >> at >> org.apache.webbeans.component.InterceptorBean.intercept(InterceptorBean.java:136) >> at >> org.apache.webbeans.intercept.InterceptorInvocationContext.proceed(InterceptorInvocationContext.java:65) >> at >> org.apache.webbeans.intercept.DefaultInterceptorHandler.invoke(DefaultInterceptorHandler.java:139) >> at >> com.tr2.engine.HotelDataSheetService$$OwbInterceptProxy0.saveDataSheet(com/tr2/engine/HotelDataSheetService.java) >> at >> com.tr2.engine.HotelDataSheetService$$OwbNormalScopeProxy0.saveDataSheet(com/tr2/engine/HotelDataSheetService.java) >> at >> com.tr2.test.TestDataRepository.saveDatasheet(TestDataRepository.java:245) >> at >> java.base/jdk.internal.reflect.NativeMethodAccessorImpl.invoke0(Native >> Method) >> at >> java.base/jdk.internal.reflect.NativeMethodAccessorImpl.invoke(NativeMethodAccessorImpl.java:62) >> at >> java.base/jdk.internal.reflect.DelegatingMethodAccessorImpl.invoke(DelegatingMethodAccessorImpl.java:43) >> at java.base/java.lang.reflect.Method.invoke(Method.java:564) >> at >> org.apache.webbeans.intercept.AbstractInvocationContext.directProceed(AbstractInvocationContext.java:113) >> at >> org.apache.webbeans.intercept.AbstractInvocationContext.proceed(AbstractInvocationContext.java:106) >> at >> org.apache.webbeans.intercept.InterceptorInvocationContext.proceed(InterceptorInvocationContext.java:78) >> at >> com.tr2.engine.base.monitoring.JavaMelodyUtil.lambda$intercept$1(JavaMelodyUtil.java:91) >> at >> com.tr2.engine.base.monitoring.JavaMelodyUtil.intercept(JavaMelodyUtil.java:54) >> at >> com.tr2.engine.base.monitoring.JavaMelodyUtil.intercept(JavaMelodyUtil.java:89) >> at >> com.tr2.engine.base.monitoring.JavaMelodyUtil.interceptAsync(JavaMelodyUtil.java:103) >> at >> com.infratr2.interceptor.async.AsyncInterceptor.asyncExecution(AsyncInterceptor.java:67) >> at >> com.infratr2.interceptor.async.AsyncInterceptor.lambda$aroundInvoke$0(AsyncInterceptor.java:47) >> at org.apache.openejb.threads.task.CUTask.invoke(CUTask.java:100) >> at org.apache.openejb.threads.task.CUCallable.call(CUCallable.java:31) >> at java.base/java.util.concurrent.FutureTask.run(FutureTask.java:264) >> at >> java.base/java.util.concurrent.ThreadPoolExecutor.runWorker(ThreadPoolExecutor.java:1130) >> at >> java.base/java.util.concurrent.ThreadPoolExecutor$Worker.run(ThreadPoolExecutor.java:630) >> at java.base/java.lang.Thread.run(Thread.java:832) >> 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) >> ... 50 more >> >> >> The test is an application composer with CDI and jpa enabled. >> >> >> >> On Wed, Jul 29, 2020 at 8:54 AM Romain Manni-Bucau <[email protected]> >> wrote: >> >>> 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? >>>>> >>>>>
