AxonFramework / cdi

Axon Framework CDI Support
Apache License 2.0
11 stars 10 forks source link

CDI Review #7

Open m-reza-rahman opened 6 years ago

m-reza-rahman commented 6 years ago

Make sure CDI is being used in the most optimal way.

hwellmann commented 6 years ago

I'm currently playing around with axon-cdi, and I just can't make it work with my non-Spring clone of the AxonBank example.

Looking at the source code of AxonCdiExtension, I think there are some major misconceptions about CDI extensions and contextual instances. You shouldn't create any contextual instances in a portable extension, and you definitely shouldn't call Configuration.start() in any of the container lifecycle event handlers.

m-reza-rahman commented 6 years ago

Feel free to submit a PR. Otherwise, feel free to create a reproducible issue you are running into. In the least a stack trace would be helpful. Thanks.

hwellmann commented 6 years ago

I don't think a PR at this stage would be meaningful.

To illustrate my points, I've created a heavily hacked fork which works with a de-Springified version of the Axon Bank example.

Please see

m-reza-rahman commented 6 years ago

OK. Let me try to look at it when time allows. Porting over Axon Bank is certainly a good idea and a pretty comprehensive test. Hopefully generating a stack trace is trivial.

zambrovski commented 6 years ago

I'm currently playing around with axon-cdi, and I just can't make it work with my non-Spring clone of the AxonBank example.

Looking at the source code of AxonCdiExtension, I think there are some major misconceptions about CDI extensions and contextual instances. You shouldn't create any contextual instances in a portable extension, and you definitely shouldn't call Configuration.start() in any of the container lifecycle event handlers.

Could you please explain what you mean by "misconception"? Why a portable extension should not create contextual instances? What is the problem about this?

m-reza-rahman commented 6 years ago

Gentlemen,

Just so you are aware, I am collaborating closely with some of the best minds in the CDI ecosystem to figure some of this out. For now suffice it to say Axon CDI enablement is not so trivial for a variety of reasons and one should not be too surprised to see things that are "unusual" for CDI extensions. Indeed the current module follows very closely what is done and proven in the Spring/Spring Boot module (as Simon is well aware). We at AxonIQ are certainly very thankful to Simon for getting this work started. I think the most important part is getting things reliably functional across containers. Precisely how that is done is and needs to be a secondary priority (and for some very good reasons - there are some things that are unexpected situations in the CDI world given that there are so few CDI extensions out there by comparison to extensions to core Spring/Spring Boot). Please give us some time to sort that out. We are working with the right people to do that already but further help, especially in terms of intelligent PRs, are certainly welcome.

Getting Axon Trader working, even if with a few reasonable compromises in the end, is certainly a valuable goal and I am glad to see Harald has taken interest and initiative to give it an early shot.

Peace, Reza

hwellmann commented 6 years ago

Could you please explain what you mean by "misconception"? Why a portable extension should not create contextual instances? What is the problem about this?

In the event observers of a CDI extension, you can modify bean definitions. Bean instances are not yet available at this stage. In fact, it wouldn't make sense for the container or your extension to create bean instances while the bean definitions are still mutable and DeploymentExceptions may still occur.

Moreover, by creating instances explicitly, you ignore the C in CDI. Contexts are not yet started during the container bootstrap phase, so any instances you create at this stage will not be associated to a context. As long as you want singleton semantics (which seems to be the case for most Axon beans), this may seem to work, but it simply does not respect the CDI lifecycle and will break graceful shutdown.

Another misconception: annotation handling.

In a CDI extension, you should never scan a class or method for annotations as in org.axonframework.cdi.CdiUtilities, you should always use javax.enterprise.inject.spi.AnnotatedType to find annotated members. In fact, CDI extension may modify annoted types by adding or removing annotations on a logical level, so you would get incorrect information by looking at the physical Java annotations only. And you should never assume that your extension is the only one - so even if you think you know what you are doing, some other extension may add (logical) annotations to the classes your extension is processing.

Another misconception: bean existence vs. producer method existence

AxonCdiExtension frequently uses the pattern

IF there is a producer method for a bean of type X 
THEN invoke it and register the result in the Configurer.

This may break for multiple reasons:

Please see my fork https://github.com/hwellmann/cdi/tree/cdi-2.0 for a more "CDI native" approach.

m-reza-rahman commented 6 years ago

I'll take a look and see if the approaches can be synced.