Open bert-laverman opened 6 years ago
EJB SLSB (Pooled) corresponds to a RequestScope of CDI. What is your proposal? Currently, the CDI scope is explicitly set to singleton (ApplicationScoped.class
is used) for any beans, which are constructed.
Bert is referring to a side-effect of our early instantiation of beans I believe. The end result is that we wind up with our own copy of an application scoped bean while once the CDI container actually starts up, it does not see this and creates another instance of the same application scoped bean (in fact this might be Weld specific behavior only). The solution I believe is to try to see if we can further defer the instantiation of beans (event listeners specifically). That may or may not be possible and we may need to live with this being an idiosyncrasy of Axon CDI integration.
One solution I am reluctant to pursue right now is abstract all bean access behind a byte code generated proxy so that references are only obtained once the application actually starts up. A PR that does this would certainly be welcome. I am happy to provide further guidance to anyone interested.
Please be careful: @ApplicationScoped does not guarantee that all beans are singletons. Only @Singleton does this. All @XxxScoped annotations are meant to indicate lifetime/statefulness, not number of instances. A @RequestScoped bean has no state beyond the lifetime of the corresponding (Web) request. @ApplicationScoped signifies the bean can be depended on to maintain state during the lifetime of the entire application.
If you look at the logging of a run of a simple application with a REST endpoint in e.g. Payara micro, you will see there are 5 threads handling requests. Give objects a unique id, and you will see 5 unique IDs in the logging. Definitely NOT singletons!
This is NOT a result, nor an unintended side-effect, of any code in the CDI module. This is how JEE works. The Application Server is free to use a pool of beans to service requests, and Payara apparently will use 5 instances by default.
As long as the CDI module forces the pre-instantiation of a single bean, and then only registers that one as an EventHandler, any subsequently instantiated beans will be ignored. If you try to prevent this behaviour by putting a @Singleton on the endpoint class, you will get an error, because the Application Server wants to be able to create a pool of handlers.
BTW there is no need to go for byte-coding or runtime generation of classes. You can hook into the CDI ProcessInjectionTarget event and replace the InjectionTarget with an anonymous class to add registration of the Bean on PostConstruct, and deregister again at PreDestroy. This does not affect the actual class that has the @Inject on a field, only the BeanManager. However, is does mean we need an event processor that can add/remove handlers after start() has been called.
OK. I'll definitely make sure to discuss this with the CDI experts and see what they think. Thanks.
Just had a small discussion with Allard on this. I'm back with my feet on the ground.
We agreed that it is fundamentally silly to put an EventHandler in a SessionScoped or even RequestScoped Bean. When an event comes in, what is the relevant scope to process it in? Which of the Beans do we select? Events only make sense in ApplicationScoped beans, unless you have recorded the original request or session where the command was sent from, and correlate the two, AND the scope is still open. The costs are however high, possibly unreasonably so. (diplomatically phrased) The obvious exception is an EventHandler in an Aggregate, but that is already taken care of.
EventHandlers put in a REST endpoint, which is the admittedly fundamental issue in my example, expose a problem, but should not be the example to validate the extension against. To get it working correctly, I moved the EventHandler to a separate Singleton Bean, and made the endpoint Inject that. My apologies that I kept yakking about the errors the code caused, while the example wasn't actually a valid use-case. The underlying discussion about the difference between bean scope and instance quantity remains valid however.
Perhaps the first iteration should simply generate a logged WARNING that an EventHandler is defined in a class which is not a Singleton. If you really want to do that, a solution exists, but it is expensive and probably not what you want anyway. The part of the problem that we SHOULD fix however, is when to instantiate beans. The extension should do that in the AfterDeploymentValidation CDI Event handler, not in the AfterBeanDiscovery handler where currently practically all work is done.
I am in general agreement I have to say. The primary use case is getting things working properly with @ApplicationScoped and maybe that's the only thing we support in the end. That is honestly what I am trying to sort out with the right folks at the moment, including the best place to do bean instantiation given what the Axon API expects and is already established in the Spring realm.
I appreciate your enthusiasm and I would still welcome a PR when you have time realizing your ideas in code. Your examples have certainly helped uncover our circular dependency problem and early initialization problem. In the end, we won't be able to control how people will wind up using the module unless we clearly document expected usage patterns (at the bare minimum). Maybe there are even ways to meet those usage patterns once we have the basics nailed down.
A JEE container typically creates several instances of an EJB or JAXB endpoint in a pool, to handle the incoming requests. The CDI code currently forces instantiation of one Bean and registers that, but the other instances will not receive any events.