smallrye / smallrye-fault-tolerance

SmallRye implementation of MicroProfile Fault Tolerance: bulkheads, circuit breakers, fallbacks, rate limits, retries, timeouts, and more
Apache License 2.0
85 stars 36 forks source link

CDI OpenWebBeans implementation incompatibility #711

Open jeanouii opened 1 year ago

jeanouii commented 1 year ago

Hi,

I was looking to use SmallRye Fault Tolerance to Apache TomEE for MicroProfile Fault Tolerance.

Unfortunatly I found that it does not work. I wanted to provide a TCK runner within a profile in SmallRye Fault Tolerance to show that it does not work in OpenWebBeans.

I checked the reason and this is because the approach used in SmallRye seems to be not fully supported by OpenWebBeans. At this moment, it is not fully clear if this is fully described in the specification. But definitely I will try to push some TCK tests and submit a PR to clarify.

Interceptor bindings are transitive—an interceptor binding declared by an interceptor binding type is inherited by all components and other interceptor binding types that declare that interceptor binding type.

An interceptor binding type can only be applied to an interceptor binding type defining a subset of its target types. For example, interceptor binding types declared Target(TYPE) may not be applied to interceptor binding types declared Target({TYPE, METHOD}).

Looks like there is a blury area between Interceptor Spec and CDI Spec. Weld supports applying InterceptorBinding on another InterceptorBinding.

It seems to be also supported in OpenWebBeans with Reflection but the specification isn't clear if annotated types should be also supported. See OpenWebbeans code https://github.com/apache/openwebbeans/blob/master/webbeans-impl/src/main/java/org/apache/webbeans/annotation/AnnotationManager.java#L231

Long story short, I used a quick hack in TomEE and I was wondering if it would make sense to use the same approach in SmallRye so that it works out of the box with Weld and OpenWebBeans or if I keep it this way in TomEE.

https://github.com/apache/tomee/blob/main/tomee/tomee-microprofile/mp-common/src/main/java/org/apache/tomee/microprofile/faulttolerance/MPFaultToleranceCDIExtension.java

Thoughts?

If you guys agree to reverse the approach then I can create a PR with a TCK runner also for OpenWebBeans.

Thanks

Ladicek commented 1 year ago

I take it the problematic thing is how our FaultToleranceExtension registers MP Fault Tolerance interceptor bindings via a custom implementation of AnnotatedType which pretends that each MP Fault Tolerance interceptor binding also has @FaultToleranceBinding?

Your solution adds @FaultToleranceBinding to all classes and methods that have any MP Fault Tolerance annotation, which requires observing PAT for all annotated types in the deployment, which has negative performance implications. I don't think we want to do that in SmallRye Fault Tolerance.

When it comes to specification clarity, I think your quote from the Interceptors spec is pretty clear: transitive interceptor bindings must work. It seems OWB's implementation of BBD.addInterceptorBinding(AnnotatedType) just doesn't search that AnnotatedType for [transitive] interceptor bindings, which in my opinion is a bug. Perhaps this TODO needs fixing? https://github.com/apache/openwebbeans/blob/master/webbeans-impl/src/main/java/org/apache/webbeans/portable/events/discovery/BeforeBeanDiscoveryImpl.java#L175

One thing we could do is switch from BBD.addInterceptorBinding(AnnotatedType) to BBD.addInterceptorBinding(Class, Annotation...). That would probably be more elegant and, looking at OWB code, have a higher chance of working there too. Do you wanna take a stab at it?

jeanouii commented 1 year ago

@Ladicek Thanks for the additional thoughts.

Yes, I can add an OpenWebBeans runner and give it a try with the other approach.

It was not clear to me whereas it was a bug or not based on the specification. But I'm happy to also fix the annotated type in OpenWebBeans anyways.

Thanks again for the additional thoughts and the peace of information.