Skip to content
This repository has been archived by the owner on Jul 11, 2019. It is now read-only.

CDI Review #7

Open
m-reza-rahman opened this issue Aug 23, 2018 · 8 comments
Open

CDI Review #7

m-reza-rahman opened this issue Aug 23, 2018 · 8 comments
Assignees

Comments

@m-reza-rahman
Copy link
Contributor

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

@hwellmann
Copy link

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
Copy link
Contributor Author

m-reza-rahman commented Sep 23, 2018

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
Copy link

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
Copy link
Contributor Author

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
Copy link

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
Copy link
Contributor Author

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
Copy link

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:

  • The user did not use a producer method at all to provide the desired type. (E.g. they created a subclass of X and added a scope annotation.)
  • There are multiple producer methods marked as @Alternative with different priorities. You will receive a ProcessProducer for each alternative, not just for the enabled one.
  • The producer method has a return type Y which extends X. This producer will create a bean with types including X and Y, but your observer for ProcessProducer<T, X> will not be called for this producer.

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

@m-reza-rahman
Copy link
Contributor Author

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

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

No branches or pull requests

4 participants