You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@tomcat.apache.org by Rémy Maucherat <re...@apache.org> on 2022/11/22 15:11:26 UTC
CDI and injection issues
Hi,
Following a post on the user list, I have looked into CDI and
injection processing in Tomcat standalone (or standalone extended by
CDI) and found the following issues:
a) metadata-complete is done wrong. The spec got retconned some time
ago and metadata-complete only means Servlet spec defining metadata,
such as @WebServlet (= the ones that require scanning all classes just
in case). So in practice inside DefaultInstanceManager the
ignoreAnnotations flag shouldn't be used at all and it should simply
be removed. I am ok on only doing this in 11 ;)
b) CDI is not intertwined with the DefaultInstanceManager. Basically
as the user said, injections should be done *before* @PostConstruct,
and it . There's a (minor) problem with the DefaultInstanceManager
API, a method needs to be protected and then integrations will then be
able to hack inside the DefaultInstanceManager. You can see this here
in the OWB integration:
https://github.com/apache/tomcat/blob/main/modules/owb/src/main/java/org/apache/webbeans/web/tomcat/OpenWebBeansInstanceManager.java#L102
(basically, first call newInstance, because there's no other way, then
inject, but newInstance will have already invoked @PostConstruct). To
fix this, I plan to add an Injector interface to
DefaultInstanceManager since this is pretty much the only way to do
this cleanly in Tomcat standalone (taking over the whole
DefaultInstanceManager is clearly not the best idea).
The impact of fixing these for users should be non-existent: it is
really a Tomcat standalone only thing impacting users with some very
precise EE needs. A full EE integration will simply take over the
whole annotation processing and instance manager from Tomcat, and
hopefully do The Right Thing already.
Although this is not super critical, I plan to address these issues in
the OWB integration (after adding the needed API change in Tomcat's
DefaultInstanceManager).
Comments ?
Rémy
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@tomcat.apache.org
For additional commands, e-mail: dev-help@tomcat.apache.org
Re: CDI and injection issues
Posted by Rémy Maucherat <re...@apache.org>.
On Tue, Nov 22, 2022 at 4:11 PM Rémy Maucherat <re...@apache.org> wrote:
>
> Hi,
>
> Following a post on the user list, I have looked into CDI and
> injection processing in Tomcat standalone (or standalone extended by
> CDI) and found the following issues:
Ok, so the summary:
> a) metadata-complete is done wrong. The spec got retconned some time
> ago and metadata-complete only means Servlet spec defining metadata,
> such as @WebServlet (= the ones that require scanning all classes just
> in case). So in practice inside DefaultInstanceManager the
> ignoreAnnotations flag shouldn't be used at all and it should simply
> be removed. I am ok on only doing this in 11 ;)
Done in 11. When a migration doc is added, ignoreAnnotations on
Context should be mentioned as the new metadata-complete.
> b) CDI is not intertwined with the DefaultInstanceManager. Basically
> as the user said, injections should be done *before* @PostConstruct,
> and it . There's a (minor) problem with the DefaultInstanceManager
> API, a method needs to be protected and then integrations will then be
> able to hack inside the DefaultInstanceManager. You can see this here
> in the OWB integration:
> https://github.com/apache/tomcat/blob/main/modules/owb/src/main/java/org/apache/webbeans/web/tomcat/OpenWebBeansInstanceManager.java#L102
> (basically, first call newInstance, because there's no other way, then
> inject, but newInstance will have already invoked @PostConstruct). To
> fix this, I plan to add an Injector interface to
> DefaultInstanceManager since this is pretty much the only way to do
> this cleanly in Tomcat standalone (taking over the whole
> DefaultInstanceManager is clearly not the best idea).
The current solution "works" with a few gotchas, so we'll simply keep
that instead.
Rémy
> The impact of fixing these for users should be non-existent: it is
> really a Tomcat standalone only thing impacting users with some very
> precise EE needs. A full EE integration will simply take over the
> whole annotation processing and instance manager from Tomcat, and
> hopefully do The Right Thing already.
>
> Although this is not super critical, I plan to address these issues in
> the OWB integration (after adding the needed API change in Tomcat's
> DefaultInstanceManager).
>
> Comments ?
>
> Rémy
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@tomcat.apache.org
For additional commands, e-mail: dev-help@tomcat.apache.org
Re: CDI and injection issues
Posted by Rémy Maucherat <re...@apache.org>.
On Thu, Nov 24, 2022 at 6:15 PM Romain Manni-Bucau
<rm...@gmail.com> wrote:
>
> Le jeu. 24 nov. 2022 à 16:58, Rémy Maucherat <re...@apache.org> a écrit :
>
> > On Thu, Nov 24, 2022 at 10:19 AM Romain Manni-Bucau
> > <rm...@gmail.com> wrote:
> > >
> > > Le jeu. 24 nov. 2022 à 10:13, Rémy Maucherat <re...@apache.org> a écrit :
> > >
> > > > On Wed, Nov 23, 2022 at 11:10 AM Romain Manni-Bucau
> > > > <rm...@gmail.com> wrote:
> > > > >
> > > > > Well, it is not that simple.
> > > > >
> > > > > Two notes on that:
> > > > >
> > > > > 1. One point is the API, injector and instance manager are the exact
> > same
> > > > > API if you want a generic API so not sure it should be duplicated
> > with
> > > > > different names (or said otherwise the Injector API is not generic
> > enough
> > > > > to be worth a tomcat "core" change IMHO)
> > > >
> > > > This is indeed the same API.
> > > > Tomcat standalone handles Jakarta Annotations here:
> > > >
> > > >
> > https://github.com/apache/tomcat/blob/main/java/org/apache/catalina/core/DefaultInstanceManager.java#L174
> > > > The owb integration is supposed to do the CDI annotations on the same
> > > > spot. AFAIK, it works. Except that the PostConstruct will be done
> > > > later.
> > > > I don't want to duplicate DefaultInstanceManager since there are a lot
> > > > of privileged actions and code in there. So adding the callback on
> > > > line 174 does seem fine to me.
> > > >
> > > > > 2. the post construct is a conflict between tomcat world and cdi
> > world
> > > > > there but you have the same "conflict" (= concurrent world/handling)
> > for
> > > > > injections, i.e. tomcat handles persistence context, persistence
> > unit,
> > > > > webservice ref etc but CDI can also handle it so the injector is
> > either
> > > > > incomplete in one case (cdi without these handling) and should be
> > > > combined
> > > > > between tomcat and injector OR it is done twice, potentially
> > differently.
> > > > >
> > > > > So at the end it looks like you don't have the choice to just own the
> > > > > instance manager to ensure it is done properly so making it easier to
> > > > > instantiate is likely the way to go but adding an abstraction which
> > is
> > > > not
> > > > > generic is maybe not helping as much as it can look like upfront.
> > > > >
> > > > > Side note: meecrowave does a more cdi instantiation:
> > > > >
> > > >
> > https://github.com/apache/openwebbeans-meecrowave/blob/master/meecrowave-core/src/main/java/org/apache/meecrowave/tomcat/CDIInstanceManager.java
> > > > > and it is very few lines so not sure the injector would help much
> > more
> > > > for
> > > > > a tomcat-cdi integration.
> > > >
> > > > Sure, but the problem is that I have to keep DefaultInstanceManager
> > > > around, and I won't replace everything with a CDI implementation.
> > > >
> > >
> > > Well, in the owb module you will have to - previous impl is more about
> > > letting cdi handling the postconstruct but lack the JNDI support to be
> > > complete - to solve the double/conflict injection point anyway. The
> > > *Injector* API does not solve it so not sure there is much space for this
> > > invalid by design API :s. Did you find a way to solve it?
> >
> > I don't understand everything in this area.
> > I tried a Servlet with a @PostConstruct. Tomcat's
> > DefaultInstanceManager invokes that method.
> > If OWB is active through the listener on that webapp, and processes
> > that Servlet instance for injection (there's nothing to inject of
> > course), then it does not invoke that @PostConstruct. Why is that ?
> > Does it only do it on beans where it actually did some injection ?
> >
>
> If you speak of the tomcat/module impl it is because it uses owb.inject
> only and not the unmanaged API (or injectiontarget which is equivalent).
> The issues start when you have such a bean (whatever component it is, a
> servlet, filter, listener):
>
> public class FooComponent ... {
> @Inject MyBean service;
> @Resource DataSource dataSource; // from web.xml for ex
> @Resource(lookup = "entries/conf)" String conf; // from context.xml for
> ex
> @PostConstruct void onInit() {}
> @PreDestroy void onDestroy() {}
> }
>
> Here you want:
>
> 1. newInstance somehow
> 2.a. inject cdi instances
> 2.b. inject tomcat instances
> 3. postconstruct
> ....
> 4. predestroy
> 5. release creation context if relevant (not normal scoped bean mainly)
>
> 2.a and b only work if you have 2 injectors (or chain the injections in
> instance manager since it is equivalent) but the trick is that OWB can
> handle tomcat injections - it has API/plugins for part of that - or tomcat
> can handle OWB injections.
> You also have the hybrid case: @Inject @Resource Foo bar; is not forbidden
> by the spec and is a valid CDI case - where an extension can make @Resource
> a qualifier.
> So at the end, the only proper way to make this working is to make the
> tomcat injections handled properly either by making tomcat aware of the CDI
> model (using annotated type for ex and using it to filter out injections to
> not do) or to just let CDI handle all injections importing the tomcat JNDI
> support in CDI - that's what TomEE does.
Ok, so I understand the argument "not really much better". So I'll
leave things as they are right now for cdi and only rework
metadata-complete.
Rémy
> But in all cases, Injector API does not solve the injection issue and it is
> likely than solving the injection issue you will end up with a
> postconstruct handling which would be a one liner (thanks CDI injection
> target for ex) and then just trivial to do in instance manager.
>
> Another not well defined case would be the exact same but using the
> constructor to get injection, this is fine for CDI but guess spec
> integration is vague enough to not handle it explicitly.
>
>
> >
> > Rémy
> >
> > >
> > >
> > > >
> > > > Rémy
> > > >
> > > > >
> > > > > 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. 23 nov. 2022 à 10:56, Rémy Maucherat <re...@apache.org> a
> > écrit :
> > > > >
> > > > > > On Tue, Nov 22, 2022 at 6:08 PM Romain Manni-Bucau
> > > > > > <rm...@gmail.com> wrote:
> > > > > > >
> > > > > > > Hmm, how is your injector different from an InstanceManager? (
> > > > > > >
> > > > > >
> > > >
> > https://github.com/apache/tomcat/blob/main/java/org/apache/tomcat/InstanceManager.java
> > > > > > > )
> > > > > > > Only by not having all `newInstance` flavors?
> > > > > >
> > > > > > The plan is to keep on using DefaultInstanceManager since OWB only
> > > > > > needs a callback for injection (before PostConstruct) and destroy
> > (to
> > > > > > remove the instance from the map). Otherwise the whole
> > InstanceManager
> > > > > > has to be replaced (and done better than the current one since the
> > > > > > PostConstruct calls cannot be in the right order otherwise).
> > > > > >
> > > > > > > Will also need the backgroundProcess (cache cleanup, same as
> > instance
> > > > > > > manager) and similarly the calling context (new instance
> > params), in
> > > > > > > particular the classloader.
> > > > > > > This is why I said it sounds more like a single API and more
> > hooking
> > > > the
> > > > > > > default instance to enable what you want or just redo it is
> > likely
> > > > > > simpler
> > > > > > > _on an API standpoint_.
> > > > > > > Guess postConstruct() method as in TomEE impl - but using tomcat
> > in
> > > > place
> > > > > > > method, just exposing it - can be sufficient and keep the API
> > clean
> > > > if
> > > > > > you
> > > > > > > don't want to reimpl anything.
> > > > > >
> > > > > > This is only a callback for injection with DefaultInstanceManager.
> > > > > > Implementing your own InstanceManager in TomEE is still needed and
> > > > > > since it is not using DefaultInstanceManager this callback API is
> > not
> > > > > > available at all. Also it will not break existing InstanceManager
> > > > > > implementations (even ones that extend DefaultInstanceManager), so
> > > > > > that's another thing I had to consider.
> > > > > >
> > > > > > The user who got me into this is here:
> > > > > > https://lists.apache.org/thread/1g4bpj3jnmmmxkh37qvwzoplqo65ldot
> > > > > > This is about Weld, but after having a look the
> > > > > > OpenWebBeansInstanceManager does the same thing. Then I realized
> > > > > > unless the InstanceManager was fully replaced there would be no
> > way to
> > > > > > implement the proper ordering.
> > > > > >
> > > > > > > About JSP hack, it is more general but hits mainly JSP: it is
> > about
> > > > > > tomcat
> > > > > > > specific JNDI injections, the workaround and wiring used
> > elsewhere
> > > > for
> > > > > > > beans didn't work properly for JSP. Guess you don't have this
> > issue
> > > > but
> > > > > > > something making it easier to handle can also probably be
> > welcomed by
> > > > > > > consumers.
> > > > > >
> > > > > > Ok.
> > > > > >
> > > > > > Rémy
> > > > > >
> > > > > > >
> > > > > > >
> > > > > > > Le mar. 22 nov. 2022 à 17:26, Rémy Maucherat <re...@apache.org> a
> > > > écrit :
> > > > > > >
> > > > > > > > On Tue, Nov 22, 2022 at 4:30 PM Romain Manni-Bucau
> > > > > > > > <rm...@gmail.com> wrote:
> > > > > > > > >
> > > > > > > > > Hi Rémy,
> > > > > > > > >
> > > > > > > > > I put a few comments inline hoping it helps
> > > > > > > > >
> > > > > > > > >
> > > > > > > > > Le mar. 22 nov. 2022 à 16:11, Rémy Maucherat <
> > remm@apache.org> a
> > > > > > écrit :
> > > > > > > > >
> > > > > > > > > > Hi,
> > > > > > > > > >
> > > > > > > > > > Following a post on the user list, I have looked into CDI
> > and
> > > > > > > > > > injection processing in Tomcat standalone (or standalone
> > > > extended
> > > > > > by
> > > > > > > > > > CDI) and found the following issues:
> > > > > > > > > >
> > > > > > > > > > a) metadata-complete is done wrong. The spec got retconned
> > some
> > > > > > time
> > > > > > > > > > ago and metadata-complete only means Servlet spec defining
> > > > > > metadata,
> > > > > > > > > > such as @WebServlet (= the ones that require scanning all
> > > > classes
> > > > > > just
> > > > > > > > > > in case). So in practice inside DefaultInstanceManager the
> > > > > > > > > > ignoreAnnotations flag shouldn't be used at all and it
> > should
> > > > > > simply
> > > > > > > > > > be removed. I am ok on only doing this in 11 ;)
> > > > > > > > > >
> > > > > > > > > > b) CDI is not intertwined with the DefaultInstanceManager.
> > > > > > Basically
> > > > > > > > > > as the user said, injections should be done *before*
> > > > > > @PostConstruct,
> > > > > > > > > > and it . There's a (minor) problem with the
> > > > DefaultInstanceManager
> > > > > > > > > > API, a method needs to be protected and then integrations
> > will
> > > > > > then be
> > > > > > > > > > able to hack inside the DefaultInstanceManager. You can see
> > > > this
> > > > > > here
> > > > > > > > > > in the OWB integration:
> > > > > > > > > >
> > > > > > > > > >
> > > > > > > >
> > > > > >
> > > >
> > https://github.com/apache/tomcat/blob/main/modules/owb/src/main/java/org/apache/webbeans/web/tomcat/OpenWebBeansInstanceManager.java#L102
> > > > > > > > > > (basically, first call newInstance, because there's no
> > other
> > > > way,
> > > > > > then
> > > > > > > > > > inject, but newInstance will have already invoked
> > > > @PostConstruct).
> > > > > > To
> > > > > > > > > > fix this, I plan to add an Injector interface to
> > > > > > > > > > DefaultInstanceManager since this is pretty much the only
> > way
> > > > to do
> > > > > > > > > > this cleanly in Tomcat standalone (taking over the whole
> > > > > > > > > > DefaultInstanceManager is clearly not the best idea).
> > > > > > > > > >
> > > > > > > > >
> > > > > > > > > Yeah, TomEE ([1]) had some hard time with JSP due to that
> > and its
> > > > > > > > > re-implementation of the instantiation.
> > > > > > > > > Basically the idea was to implement it aside Tomcat default
> > impl
> > > > but
> > > > > > for
> > > > > > > > > some classes it was not convenient enough.
> > > > > > > > > That said I have to admit I'm not sure it needs a new concept
> > > > > > (injector)
> > > > > > > > > because basically it will copy the instance manager API
> > (inject,
> > > > > > release
> > > > > > > > or
> > > > > > > > > something like that since it is not only about the inject
> > phase
> > > > so
> > > > > > either
> > > > > > > > > you define the lifecycle or you store a Runnable you call at
> > > > release
> > > > > > time
> > > > > > > > > too - isnt the abstraction too complex then?).
> > > > > > > >
> > > > > > > > For EE or some other similar embedded, the assumption was that
> > > > > > > > InstanceManager would be fully reimplemented. But this is more
> > work
> > > > > > > > for a lighter weight integration, ok.
> > > > > > > >
> > > > > > > > The change to add the API to DefaultInstanceManager would be:
> > > > > > > > --- a/java/org/apache/catalina/core/DefaultInstanceManager.java
> > > > > > > > +++ b/java/org/apache/catalina/core/DefaultInstanceManager.java
> > > > > > > > @@ -109,6 +109,7 @@ public class DefaultInstanceManager
> > implements
> > > > > > > > InstanceManager {
> > > > > > > > new ManagedConcurrentWeakHashMap<>();
> > > > > > > > private final Map<String, String> postConstructMethods;
> > > > > > > > private final Map<String, String> preDestroyMethods;
> > > > > > > > + private Injector injector = null;
> > > > > > > >
> > > > > > > > public DefaultInstanceManager(Context context,
> > > > > > > > Map<String, Map<String, String>> injectionMap,
> > > > > > > > @@ -172,6 +173,9 @@ public class DefaultInstanceManager
> > implements
> > > > > > > > InstanceManager {
> > > > > > > > Map<String, String> injections =
> > > > > > > > assembleInjectionsFromClassHierarchy(clazz);
> > > > > > > > populateAnnotationsCache(clazz, injections);
> > > > > > > > processAnnotations(instance, injections);
> > > > > > > > + if (injector != null) {
> > > > > > > > + injector.inject(instance);
> > > > > > > > + }
> > > > > > > > postConstruct(instance, clazz);
> > > > > > > > }
> > > > > > > > return instance;
> > > > > > > > @@ -193,6 +197,9 @@ public class DefaultInstanceManager
> > implements
> > > > > > > > InstanceManager {
> > > > > > > > @Override
> > > > > > > > public void destroyInstance(Object instance) throws
> > > > > > > > IllegalAccessException,
> > > > > > > > InvocationTargetException {
> > > > > > > > + if (injector != null) {
> > > > > > > > + injector.destroy(instance);
> > > > > > > > + }
> > > > > > > > if (!ignoreAnnotations) {
> > > > > > > > preDestroy(instance, instance.getClass());
> > > > > > > > }
> > > > > > > > @@ -828,4 +835,16 @@ public class DefaultInstanceManager
> > implements
> > > > > > > > InstanceManager {
> > > > > > > > return loadClass(className, classLoader);
> > > > > > > > }
> > > > > > > > }
> > > > > > > > +
> > > > > > > > + public void setInjector(Injector injector) {
> > > > > > > > + if (injector == null) {
> > > > > > > > + this.injector = injector;
> > > > > > > > + }
> > > > > > > > + }
> > > > > > > > +
> > > > > > > > + public interface Injector {
> > > > > > > > + void inject(Object instance);
> > > > > > > > + void destroy(Object instance);
> > > > > > > > + }
> > > > > > > > +
> > > > > > > > }
> > > > > > > >
> > > > > > > > Simply allowing inject and destroy callbacks on the main
> > > > > > > > DefaultInstanceManager, which is what the OWBInstanceManager
> > > > > > > > integration does (except it cannot do it in the right order for
> > > > > > > > injection, which is what the user complained about).
> > > > > > > >
> > > > > > > > > Another issue there is that newInstance assumes a "new" which
> > > > means
> > > > > > you
> > > > > > > > can
> > > > > > > > > not use a CDI instance generally speaking, only a banalised
> > > > instance
> > > > > > > > which
> > > > > > > > > gets injections so it means you can just impl yourself a
> > > > > > > > > newInstance+whatever you want+postconstruct call (this is
> > light
> > > > > > > > actually).
> > > > > > > > > This is where setting ignoreAnnotations would be quite fancy
> > in
> > > > your
> > > > > > own
> > > > > > > > > instance manager and wouldnt need any new API.
> > > > > > > >
> > > > > > > > Here it clearly relies on the behavior of
> > DefaultInstanceManager.
> > > > > > > > OTOH, maybe it's good to keep ignoreAnnotations as a real
> > "ignore
> > > > all
> > > > > > > > annotations I know what I am doing", but decouple it from the
> > > > > > > > matadata-complete from EE.
> > > > > > > >
> > > > > > > > > That said, enabling to use a CDI instance would be way more
> > > > powerful
> > > > > > and
> > > > > > > > is
> > > > > > > > > not against the spec - it is actually not specified AFAIK.
> > > > > > > > > Indeed it would need a toggle on the OWBIM of Tomcat
> > integration
> > > > but
> > > > > > it
> > > > > > > > > would also open way more doors in terms of usage because your
> > > > servlet
> > > > > > > > > (filter, ...) is now a CDI beans with a connection to its
> > bus and
> > > > > > > > > interceptors.
> > > > > > > > > I know it can already be done by using a container
> > initializer
> > > > which
> > > > > > gets
> > > > > > > > > beans injected and the instances directly passed to the
> > > > addServlet()
> > > > > > > > > (instead of the class) but it would also be a very valuable
> > > > addition
> > > > > > to
> > > > > > > > the
> > > > > > > > > module if the instantiation is reworked so I'm just
> > mentionning
> > > > it
> > > > > > as an
> > > > > > > > > opportunity.
> > > > > > > > >
> > > > > > > > >
> > > > > > > > > [1]
> > > > > > > > >
> > > > > > > >
> > > > > >
> > > >
> > https://github.com/apache/tomee/blob/main/tomee/tomee-catalina/src/main/java/org/apache/tomee/catalina/JavaeeInstanceManager.java
> > > > > > > >
> > > > > > > > Looking at the code, it seems reasonable to have a custom
> > instance
> > > > > > > > manager for TomEE as it does more than simply inject. About JSP
> > > > > > > > specifically, I'm not sure I understand the problem but that's
> > ok.
> > > > > > > >
> > > > > > > > Rémy
> > > > > > > >
> > > > > > > > >
> > > > > > > > >
> > > > > > > > > >
> > > > > > > > > > The impact of fixing these for users should be
> > non-existent:
> > > > it is
> > > > > > > > > > really a Tomcat standalone only thing impacting users with
> > some
> > > > > > very
> > > > > > > > > > precise EE needs. A full EE integration will simply take
> > over
> > > > the
> > > > > > > > > > whole annotation processing and instance manager from
> > Tomcat,
> > > > and
> > > > > > > > > > hopefully do The Right Thing already.
> > > > > > > > > >
> > > > > > > > > > Although this is not super critical, I plan to address
> > these
> > > > > > issues in
> > > > > > > > > > the OWB integration (after adding the needed API change in
> > > > Tomcat's
> > > > > > > > > > DefaultInstanceManager).
> > > > > > > > > >
> > > > > > > > > > Comments ?
> > > > > > > > > >
> > > > > > > > > > Rémy
> > > > > > > > > >
> > > > > > > > > >
> > > > > >
> > ---------------------------------------------------------------------
> > > > > > > > > > To unsubscribe, e-mail: dev-unsubscribe@tomcat.apache.org
> > > > > > > > > > For additional commands, e-mail:
> > dev-help@tomcat.apache.org
> > > > > > > > > >
> > > > > > > > > >
> > > > > > > >
> > > > > > > >
> > > > ---------------------------------------------------------------------
> > > > > > > > To unsubscribe, e-mail: dev-unsubscribe@tomcat.apache.org
> > > > > > > > For additional commands, e-mail: dev-help@tomcat.apache.org
> > > > > > > >
> > > > > > > >
> > > > > >
> > > > > >
> > ---------------------------------------------------------------------
> > > > > > To unsubscribe, e-mail: dev-unsubscribe@tomcat.apache.org
> > > > > > For additional commands, e-mail: dev-help@tomcat.apache.org
> > > > > >
> > > > > >
> > > >
> > > > ---------------------------------------------------------------------
> > > > To unsubscribe, e-mail: dev-unsubscribe@tomcat.apache.org
> > > > For additional commands, e-mail: dev-help@tomcat.apache.org
> > > >
> > > >
> >
> > ---------------------------------------------------------------------
> > To unsubscribe, e-mail: dev-unsubscribe@tomcat.apache.org
> > For additional commands, e-mail: dev-help@tomcat.apache.org
> >
> >
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@tomcat.apache.org
For additional commands, e-mail: dev-help@tomcat.apache.org
Re: CDI and injection issues
Posted by Romain Manni-Bucau <rm...@gmail.com>.
Le jeu. 24 nov. 2022 à 16:58, Rémy Maucherat <re...@apache.org> a écrit :
> On Thu, Nov 24, 2022 at 10:19 AM Romain Manni-Bucau
> <rm...@gmail.com> wrote:
> >
> > Le jeu. 24 nov. 2022 à 10:13, Rémy Maucherat <re...@apache.org> a écrit :
> >
> > > On Wed, Nov 23, 2022 at 11:10 AM Romain Manni-Bucau
> > > <rm...@gmail.com> wrote:
> > > >
> > > > Well, it is not that simple.
> > > >
> > > > Two notes on that:
> > > >
> > > > 1. One point is the API, injector and instance manager are the exact
> same
> > > > API if you want a generic API so not sure it should be duplicated
> with
> > > > different names (or said otherwise the Injector API is not generic
> enough
> > > > to be worth a tomcat "core" change IMHO)
> > >
> > > This is indeed the same API.
> > > Tomcat standalone handles Jakarta Annotations here:
> > >
> > >
> https://github.com/apache/tomcat/blob/main/java/org/apache/catalina/core/DefaultInstanceManager.java#L174
> > > The owb integration is supposed to do the CDI annotations on the same
> > > spot. AFAIK, it works. Except that the PostConstruct will be done
> > > later.
> > > I don't want to duplicate DefaultInstanceManager since there are a lot
> > > of privileged actions and code in there. So adding the callback on
> > > line 174 does seem fine to me.
> > >
> > > > 2. the post construct is a conflict between tomcat world and cdi
> world
> > > > there but you have the same "conflict" (= concurrent world/handling)
> for
> > > > injections, i.e. tomcat handles persistence context, persistence
> unit,
> > > > webservice ref etc but CDI can also handle it so the injector is
> either
> > > > incomplete in one case (cdi without these handling) and should be
> > > combined
> > > > between tomcat and injector OR it is done twice, potentially
> differently.
> > > >
> > > > So at the end it looks like you don't have the choice to just own the
> > > > instance manager to ensure it is done properly so making it easier to
> > > > instantiate is likely the way to go but adding an abstraction which
> is
> > > not
> > > > generic is maybe not helping as much as it can look like upfront.
> > > >
> > > > Side note: meecrowave does a more cdi instantiation:
> > > >
> > >
> https://github.com/apache/openwebbeans-meecrowave/blob/master/meecrowave-core/src/main/java/org/apache/meecrowave/tomcat/CDIInstanceManager.java
> > > > and it is very few lines so not sure the injector would help much
> more
> > > for
> > > > a tomcat-cdi integration.
> > >
> > > Sure, but the problem is that I have to keep DefaultInstanceManager
> > > around, and I won't replace everything with a CDI implementation.
> > >
> >
> > Well, in the owb module you will have to - previous impl is more about
> > letting cdi handling the postconstruct but lack the JNDI support to be
> > complete - to solve the double/conflict injection point anyway. The
> > *Injector* API does not solve it so not sure there is much space for this
> > invalid by design API :s. Did you find a way to solve it?
>
> I don't understand everything in this area.
> I tried a Servlet with a @PostConstruct. Tomcat's
> DefaultInstanceManager invokes that method.
> If OWB is active through the listener on that webapp, and processes
> that Servlet instance for injection (there's nothing to inject of
> course), then it does not invoke that @PostConstruct. Why is that ?
> Does it only do it on beans where it actually did some injection ?
>
If you speak of the tomcat/module impl it is because it uses owb.inject
only and not the unmanaged API (or injectiontarget which is equivalent).
The issues start when you have such a bean (whatever component it is, a
servlet, filter, listener):
public class FooComponent ... {
@Inject MyBean service;
@Resource DataSource dataSource; // from web.xml for ex
@Resource(lookup = "entries/conf)" String conf; // from context.xml for
ex
@PostConstruct void onInit() {}
@PreDestroy void onDestroy() {}
}
Here you want:
1. newInstance somehow
2.a. inject cdi instances
2.b. inject tomcat instances
3. postconstruct
....
4. predestroy
5. release creation context if relevant (not normal scoped bean mainly)
2.a and b only work if you have 2 injectors (or chain the injections in
instance manager since it is equivalent) but the trick is that OWB can
handle tomcat injections - it has API/plugins for part of that - or tomcat
can handle OWB injections.
You also have the hybrid case: @Inject @Resource Foo bar; is not forbidden
by the spec and is a valid CDI case - where an extension can make @Resource
a qualifier.
So at the end, the only proper way to make this working is to make the
tomcat injections handled properly either by making tomcat aware of the CDI
model (using annotated type for ex and using it to filter out injections to
not do) or to just let CDI handle all injections importing the tomcat JNDI
support in CDI - that's what TomEE does.
But in all cases, Injector API does not solve the injection issue and it is
likely than solving the injection issue you will end up with a
postconstruct handling which would be a one liner (thanks CDI injection
target for ex) and then just trivial to do in instance manager.
Another not well defined case would be the exact same but using the
constructor to get injection, this is fine for CDI but guess spec
integration is vague enough to not handle it explicitly.
>
> Rémy
>
> >
> >
> > >
> > > Rémy
> > >
> > > >
> > > > 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. 23 nov. 2022 à 10:56, Rémy Maucherat <re...@apache.org> a
> écrit :
> > > >
> > > > > On Tue, Nov 22, 2022 at 6:08 PM Romain Manni-Bucau
> > > > > <rm...@gmail.com> wrote:
> > > > > >
> > > > > > Hmm, how is your injector different from an InstanceManager? (
> > > > > >
> > > > >
> > >
> https://github.com/apache/tomcat/blob/main/java/org/apache/tomcat/InstanceManager.java
> > > > > > )
> > > > > > Only by not having all `newInstance` flavors?
> > > > >
> > > > > The plan is to keep on using DefaultInstanceManager since OWB only
> > > > > needs a callback for injection (before PostConstruct) and destroy
> (to
> > > > > remove the instance from the map). Otherwise the whole
> InstanceManager
> > > > > has to be replaced (and done better than the current one since the
> > > > > PostConstruct calls cannot be in the right order otherwise).
> > > > >
> > > > > > Will also need the backgroundProcess (cache cleanup, same as
> instance
> > > > > > manager) and similarly the calling context (new instance
> params), in
> > > > > > particular the classloader.
> > > > > > This is why I said it sounds more like a single API and more
> hooking
> > > the
> > > > > > default instance to enable what you want or just redo it is
> likely
> > > > > simpler
> > > > > > _on an API standpoint_.
> > > > > > Guess postConstruct() method as in TomEE impl - but using tomcat
> in
> > > place
> > > > > > method, just exposing it - can be sufficient and keep the API
> clean
> > > if
> > > > > you
> > > > > > don't want to reimpl anything.
> > > > >
> > > > > This is only a callback for injection with DefaultInstanceManager.
> > > > > Implementing your own InstanceManager in TomEE is still needed and
> > > > > since it is not using DefaultInstanceManager this callback API is
> not
> > > > > available at all. Also it will not break existing InstanceManager
> > > > > implementations (even ones that extend DefaultInstanceManager), so
> > > > > that's another thing I had to consider.
> > > > >
> > > > > The user who got me into this is here:
> > > > > https://lists.apache.org/thread/1g4bpj3jnmmmxkh37qvwzoplqo65ldot
> > > > > This is about Weld, but after having a look the
> > > > > OpenWebBeansInstanceManager does the same thing. Then I realized
> > > > > unless the InstanceManager was fully replaced there would be no
> way to
> > > > > implement the proper ordering.
> > > > >
> > > > > > About JSP hack, it is more general but hits mainly JSP: it is
> about
> > > > > tomcat
> > > > > > specific JNDI injections, the workaround and wiring used
> elsewhere
> > > for
> > > > > > beans didn't work properly for JSP. Guess you don't have this
> issue
> > > but
> > > > > > something making it easier to handle can also probably be
> welcomed by
> > > > > > consumers.
> > > > >
> > > > > Ok.
> > > > >
> > > > > Rémy
> > > > >
> > > > > >
> > > > > >
> > > > > > Le mar. 22 nov. 2022 à 17:26, Rémy Maucherat <re...@apache.org> a
> > > écrit :
> > > > > >
> > > > > > > On Tue, Nov 22, 2022 at 4:30 PM Romain Manni-Bucau
> > > > > > > <rm...@gmail.com> wrote:
> > > > > > > >
> > > > > > > > Hi Rémy,
> > > > > > > >
> > > > > > > > I put a few comments inline hoping it helps
> > > > > > > >
> > > > > > > >
> > > > > > > > Le mar. 22 nov. 2022 à 16:11, Rémy Maucherat <
> remm@apache.org> a
> > > > > écrit :
> > > > > > > >
> > > > > > > > > Hi,
> > > > > > > > >
> > > > > > > > > Following a post on the user list, I have looked into CDI
> and
> > > > > > > > > injection processing in Tomcat standalone (or standalone
> > > extended
> > > > > by
> > > > > > > > > CDI) and found the following issues:
> > > > > > > > >
> > > > > > > > > a) metadata-complete is done wrong. The spec got retconned
> some
> > > > > time
> > > > > > > > > ago and metadata-complete only means Servlet spec defining
> > > > > metadata,
> > > > > > > > > such as @WebServlet (= the ones that require scanning all
> > > classes
> > > > > just
> > > > > > > > > in case). So in practice inside DefaultInstanceManager the
> > > > > > > > > ignoreAnnotations flag shouldn't be used at all and it
> should
> > > > > simply
> > > > > > > > > be removed. I am ok on only doing this in 11 ;)
> > > > > > > > >
> > > > > > > > > b) CDI is not intertwined with the DefaultInstanceManager.
> > > > > Basically
> > > > > > > > > as the user said, injections should be done *before*
> > > > > @PostConstruct,
> > > > > > > > > and it . There's a (minor) problem with the
> > > DefaultInstanceManager
> > > > > > > > > API, a method needs to be protected and then integrations
> will
> > > > > then be
> > > > > > > > > able to hack inside the DefaultInstanceManager. You can see
> > > this
> > > > > here
> > > > > > > > > in the OWB integration:
> > > > > > > > >
> > > > > > > > >
> > > > > > >
> > > > >
> > >
> https://github.com/apache/tomcat/blob/main/modules/owb/src/main/java/org/apache/webbeans/web/tomcat/OpenWebBeansInstanceManager.java#L102
> > > > > > > > > (basically, first call newInstance, because there's no
> other
> > > way,
> > > > > then
> > > > > > > > > inject, but newInstance will have already invoked
> > > @PostConstruct).
> > > > > To
> > > > > > > > > fix this, I plan to add an Injector interface to
> > > > > > > > > DefaultInstanceManager since this is pretty much the only
> way
> > > to do
> > > > > > > > > this cleanly in Tomcat standalone (taking over the whole
> > > > > > > > > DefaultInstanceManager is clearly not the best idea).
> > > > > > > > >
> > > > > > > >
> > > > > > > > Yeah, TomEE ([1]) had some hard time with JSP due to that
> and its
> > > > > > > > re-implementation of the instantiation.
> > > > > > > > Basically the idea was to implement it aside Tomcat default
> impl
> > > but
> > > > > for
> > > > > > > > some classes it was not convenient enough.
> > > > > > > > That said I have to admit I'm not sure it needs a new concept
> > > > > (injector)
> > > > > > > > because basically it will copy the instance manager API
> (inject,
> > > > > release
> > > > > > > or
> > > > > > > > something like that since it is not only about the inject
> phase
> > > so
> > > > > either
> > > > > > > > you define the lifecycle or you store a Runnable you call at
> > > release
> > > > > time
> > > > > > > > too - isnt the abstraction too complex then?).
> > > > > > >
> > > > > > > For EE or some other similar embedded, the assumption was that
> > > > > > > InstanceManager would be fully reimplemented. But this is more
> work
> > > > > > > for a lighter weight integration, ok.
> > > > > > >
> > > > > > > The change to add the API to DefaultInstanceManager would be:
> > > > > > > --- a/java/org/apache/catalina/core/DefaultInstanceManager.java
> > > > > > > +++ b/java/org/apache/catalina/core/DefaultInstanceManager.java
> > > > > > > @@ -109,6 +109,7 @@ public class DefaultInstanceManager
> implements
> > > > > > > InstanceManager {
> > > > > > > new ManagedConcurrentWeakHashMap<>();
> > > > > > > private final Map<String, String> postConstructMethods;
> > > > > > > private final Map<String, String> preDestroyMethods;
> > > > > > > + private Injector injector = null;
> > > > > > >
> > > > > > > public DefaultInstanceManager(Context context,
> > > > > > > Map<String, Map<String, String>> injectionMap,
> > > > > > > @@ -172,6 +173,9 @@ public class DefaultInstanceManager
> implements
> > > > > > > InstanceManager {
> > > > > > > Map<String, String> injections =
> > > > > > > assembleInjectionsFromClassHierarchy(clazz);
> > > > > > > populateAnnotationsCache(clazz, injections);
> > > > > > > processAnnotations(instance, injections);
> > > > > > > + if (injector != null) {
> > > > > > > + injector.inject(instance);
> > > > > > > + }
> > > > > > > postConstruct(instance, clazz);
> > > > > > > }
> > > > > > > return instance;
> > > > > > > @@ -193,6 +197,9 @@ public class DefaultInstanceManager
> implements
> > > > > > > InstanceManager {
> > > > > > > @Override
> > > > > > > public void destroyInstance(Object instance) throws
> > > > > > > IllegalAccessException,
> > > > > > > InvocationTargetException {
> > > > > > > + if (injector != null) {
> > > > > > > + injector.destroy(instance);
> > > > > > > + }
> > > > > > > if (!ignoreAnnotations) {
> > > > > > > preDestroy(instance, instance.getClass());
> > > > > > > }
> > > > > > > @@ -828,4 +835,16 @@ public class DefaultInstanceManager
> implements
> > > > > > > InstanceManager {
> > > > > > > return loadClass(className, classLoader);
> > > > > > > }
> > > > > > > }
> > > > > > > +
> > > > > > > + public void setInjector(Injector injector) {
> > > > > > > + if (injector == null) {
> > > > > > > + this.injector = injector;
> > > > > > > + }
> > > > > > > + }
> > > > > > > +
> > > > > > > + public interface Injector {
> > > > > > > + void inject(Object instance);
> > > > > > > + void destroy(Object instance);
> > > > > > > + }
> > > > > > > +
> > > > > > > }
> > > > > > >
> > > > > > > Simply allowing inject and destroy callbacks on the main
> > > > > > > DefaultInstanceManager, which is what the OWBInstanceManager
> > > > > > > integration does (except it cannot do it in the right order for
> > > > > > > injection, which is what the user complained about).
> > > > > > >
> > > > > > > > Another issue there is that newInstance assumes a "new" which
> > > means
> > > > > you
> > > > > > > can
> > > > > > > > not use a CDI instance generally speaking, only a banalised
> > > instance
> > > > > > > which
> > > > > > > > gets injections so it means you can just impl yourself a
> > > > > > > > newInstance+whatever you want+postconstruct call (this is
> light
> > > > > > > actually).
> > > > > > > > This is where setting ignoreAnnotations would be quite fancy
> in
> > > your
> > > > > own
> > > > > > > > instance manager and wouldnt need any new API.
> > > > > > >
> > > > > > > Here it clearly relies on the behavior of
> DefaultInstanceManager.
> > > > > > > OTOH, maybe it's good to keep ignoreAnnotations as a real
> "ignore
> > > all
> > > > > > > annotations I know what I am doing", but decouple it from the
> > > > > > > matadata-complete from EE.
> > > > > > >
> > > > > > > > That said, enabling to use a CDI instance would be way more
> > > powerful
> > > > > and
> > > > > > > is
> > > > > > > > not against the spec - it is actually not specified AFAIK.
> > > > > > > > Indeed it would need a toggle on the OWBIM of Tomcat
> integration
> > > but
> > > > > it
> > > > > > > > would also open way more doors in terms of usage because your
> > > servlet
> > > > > > > > (filter, ...) is now a CDI beans with a connection to its
> bus and
> > > > > > > > interceptors.
> > > > > > > > I know it can already be done by using a container
> initializer
> > > which
> > > > > gets
> > > > > > > > beans injected and the instances directly passed to the
> > > addServlet()
> > > > > > > > (instead of the class) but it would also be a very valuable
> > > addition
> > > > > to
> > > > > > > the
> > > > > > > > module if the instantiation is reworked so I'm just
> mentionning
> > > it
> > > > > as an
> > > > > > > > opportunity.
> > > > > > > >
> > > > > > > >
> > > > > > > > [1]
> > > > > > > >
> > > > > > >
> > > > >
> > >
> https://github.com/apache/tomee/blob/main/tomee/tomee-catalina/src/main/java/org/apache/tomee/catalina/JavaeeInstanceManager.java
> > > > > > >
> > > > > > > Looking at the code, it seems reasonable to have a custom
> instance
> > > > > > > manager for TomEE as it does more than simply inject. About JSP
> > > > > > > specifically, I'm not sure I understand the problem but that's
> ok.
> > > > > > >
> > > > > > > Rémy
> > > > > > >
> > > > > > > >
> > > > > > > >
> > > > > > > > >
> > > > > > > > > The impact of fixing these for users should be
> non-existent:
> > > it is
> > > > > > > > > really a Tomcat standalone only thing impacting users with
> some
> > > > > very
> > > > > > > > > precise EE needs. A full EE integration will simply take
> over
> > > the
> > > > > > > > > whole annotation processing and instance manager from
> Tomcat,
> > > and
> > > > > > > > > hopefully do The Right Thing already.
> > > > > > > > >
> > > > > > > > > Although this is not super critical, I plan to address
> these
> > > > > issues in
> > > > > > > > > the OWB integration (after adding the needed API change in
> > > Tomcat's
> > > > > > > > > DefaultInstanceManager).
> > > > > > > > >
> > > > > > > > > Comments ?
> > > > > > > > >
> > > > > > > > > Rémy
> > > > > > > > >
> > > > > > > > >
> > > > >
> ---------------------------------------------------------------------
> > > > > > > > > To unsubscribe, e-mail: dev-unsubscribe@tomcat.apache.org
> > > > > > > > > For additional commands, e-mail:
> dev-help@tomcat.apache.org
> > > > > > > > >
> > > > > > > > >
> > > > > > >
> > > > > > >
> > > ---------------------------------------------------------------------
> > > > > > > To unsubscribe, e-mail: dev-unsubscribe@tomcat.apache.org
> > > > > > > For additional commands, e-mail: dev-help@tomcat.apache.org
> > > > > > >
> > > > > > >
> > > > >
> > > > >
> ---------------------------------------------------------------------
> > > > > To unsubscribe, e-mail: dev-unsubscribe@tomcat.apache.org
> > > > > For additional commands, e-mail: dev-help@tomcat.apache.org
> > > > >
> > > > >
> > >
> > > ---------------------------------------------------------------------
> > > To unsubscribe, e-mail: dev-unsubscribe@tomcat.apache.org
> > > For additional commands, e-mail: dev-help@tomcat.apache.org
> > >
> > >
>
> ---------------------------------------------------------------------
> To unsubscribe, e-mail: dev-unsubscribe@tomcat.apache.org
> For additional commands, e-mail: dev-help@tomcat.apache.org
>
>
Re: CDI and injection issues
Posted by Rémy Maucherat <re...@apache.org>.
On Thu, Nov 24, 2022 at 10:19 AM Romain Manni-Bucau
<rm...@gmail.com> wrote:
>
> Le jeu. 24 nov. 2022 à 10:13, Rémy Maucherat <re...@apache.org> a écrit :
>
> > On Wed, Nov 23, 2022 at 11:10 AM Romain Manni-Bucau
> > <rm...@gmail.com> wrote:
> > >
> > > Well, it is not that simple.
> > >
> > > Two notes on that:
> > >
> > > 1. One point is the API, injector and instance manager are the exact same
> > > API if you want a generic API so not sure it should be duplicated with
> > > different names (or said otherwise the Injector API is not generic enough
> > > to be worth a tomcat "core" change IMHO)
> >
> > This is indeed the same API.
> > Tomcat standalone handles Jakarta Annotations here:
> >
> > https://github.com/apache/tomcat/blob/main/java/org/apache/catalina/core/DefaultInstanceManager.java#L174
> > The owb integration is supposed to do the CDI annotations on the same
> > spot. AFAIK, it works. Except that the PostConstruct will be done
> > later.
> > I don't want to duplicate DefaultInstanceManager since there are a lot
> > of privileged actions and code in there. So adding the callback on
> > line 174 does seem fine to me.
> >
> > > 2. the post construct is a conflict between tomcat world and cdi world
> > > there but you have the same "conflict" (= concurrent world/handling) for
> > > injections, i.e. tomcat handles persistence context, persistence unit,
> > > webservice ref etc but CDI can also handle it so the injector is either
> > > incomplete in one case (cdi without these handling) and should be
> > combined
> > > between tomcat and injector OR it is done twice, potentially differently.
> > >
> > > So at the end it looks like you don't have the choice to just own the
> > > instance manager to ensure it is done properly so making it easier to
> > > instantiate is likely the way to go but adding an abstraction which is
> > not
> > > generic is maybe not helping as much as it can look like upfront.
> > >
> > > Side note: meecrowave does a more cdi instantiation:
> > >
> > https://github.com/apache/openwebbeans-meecrowave/blob/master/meecrowave-core/src/main/java/org/apache/meecrowave/tomcat/CDIInstanceManager.java
> > > and it is very few lines so not sure the injector would help much more
> > for
> > > a tomcat-cdi integration.
> >
> > Sure, but the problem is that I have to keep DefaultInstanceManager
> > around, and I won't replace everything with a CDI implementation.
> >
>
> Well, in the owb module you will have to - previous impl is more about
> letting cdi handling the postconstruct but lack the JNDI support to be
> complete - to solve the double/conflict injection point anyway. The
> *Injector* API does not solve it so not sure there is much space for this
> invalid by design API :s. Did you find a way to solve it?
I don't understand everything in this area.
I tried a Servlet with a @PostConstruct. Tomcat's
DefaultInstanceManager invokes that method.
If OWB is active through the listener on that webapp, and processes
that Servlet instance for injection (there's nothing to inject of
course), then it does not invoke that @PostConstruct. Why is that ?
Does it only do it on beans where it actually did some injection ?
Rémy
>
>
> >
> > Rémy
> >
> > >
> > > 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. 23 nov. 2022 à 10:56, Rémy Maucherat <re...@apache.org> a écrit :
> > >
> > > > On Tue, Nov 22, 2022 at 6:08 PM Romain Manni-Bucau
> > > > <rm...@gmail.com> wrote:
> > > > >
> > > > > Hmm, how is your injector different from an InstanceManager? (
> > > > >
> > > >
> > https://github.com/apache/tomcat/blob/main/java/org/apache/tomcat/InstanceManager.java
> > > > > )
> > > > > Only by not having all `newInstance` flavors?
> > > >
> > > > The plan is to keep on using DefaultInstanceManager since OWB only
> > > > needs a callback for injection (before PostConstruct) and destroy (to
> > > > remove the instance from the map). Otherwise the whole InstanceManager
> > > > has to be replaced (and done better than the current one since the
> > > > PostConstruct calls cannot be in the right order otherwise).
> > > >
> > > > > Will also need the backgroundProcess (cache cleanup, same as instance
> > > > > manager) and similarly the calling context (new instance params), in
> > > > > particular the classloader.
> > > > > This is why I said it sounds more like a single API and more hooking
> > the
> > > > > default instance to enable what you want or just redo it is likely
> > > > simpler
> > > > > _on an API standpoint_.
> > > > > Guess postConstruct() method as in TomEE impl - but using tomcat in
> > place
> > > > > method, just exposing it - can be sufficient and keep the API clean
> > if
> > > > you
> > > > > don't want to reimpl anything.
> > > >
> > > > This is only a callback for injection with DefaultInstanceManager.
> > > > Implementing your own InstanceManager in TomEE is still needed and
> > > > since it is not using DefaultInstanceManager this callback API is not
> > > > available at all. Also it will not break existing InstanceManager
> > > > implementations (even ones that extend DefaultInstanceManager), so
> > > > that's another thing I had to consider.
> > > >
> > > > The user who got me into this is here:
> > > > https://lists.apache.org/thread/1g4bpj3jnmmmxkh37qvwzoplqo65ldot
> > > > This is about Weld, but after having a look the
> > > > OpenWebBeansInstanceManager does the same thing. Then I realized
> > > > unless the InstanceManager was fully replaced there would be no way to
> > > > implement the proper ordering.
> > > >
> > > > > About JSP hack, it is more general but hits mainly JSP: it is about
> > > > tomcat
> > > > > specific JNDI injections, the workaround and wiring used elsewhere
> > for
> > > > > beans didn't work properly for JSP. Guess you don't have this issue
> > but
> > > > > something making it easier to handle can also probably be welcomed by
> > > > > consumers.
> > > >
> > > > Ok.
> > > >
> > > > Rémy
> > > >
> > > > >
> > > > >
> > > > > Le mar. 22 nov. 2022 à 17:26, Rémy Maucherat <re...@apache.org> a
> > écrit :
> > > > >
> > > > > > On Tue, Nov 22, 2022 at 4:30 PM Romain Manni-Bucau
> > > > > > <rm...@gmail.com> wrote:
> > > > > > >
> > > > > > > Hi Rémy,
> > > > > > >
> > > > > > > I put a few comments inline hoping it helps
> > > > > > >
> > > > > > >
> > > > > > > Le mar. 22 nov. 2022 à 16:11, Rémy Maucherat <re...@apache.org> a
> > > > écrit :
> > > > > > >
> > > > > > > > Hi,
> > > > > > > >
> > > > > > > > Following a post on the user list, I have looked into CDI and
> > > > > > > > injection processing in Tomcat standalone (or standalone
> > extended
> > > > by
> > > > > > > > CDI) and found the following issues:
> > > > > > > >
> > > > > > > > a) metadata-complete is done wrong. The spec got retconned some
> > > > time
> > > > > > > > ago and metadata-complete only means Servlet spec defining
> > > > metadata,
> > > > > > > > such as @WebServlet (= the ones that require scanning all
> > classes
> > > > just
> > > > > > > > in case). So in practice inside DefaultInstanceManager the
> > > > > > > > ignoreAnnotations flag shouldn't be used at all and it should
> > > > simply
> > > > > > > > be removed. I am ok on only doing this in 11 ;)
> > > > > > > >
> > > > > > > > b) CDI is not intertwined with the DefaultInstanceManager.
> > > > Basically
> > > > > > > > as the user said, injections should be done *before*
> > > > @PostConstruct,
> > > > > > > > and it . There's a (minor) problem with the
> > DefaultInstanceManager
> > > > > > > > API, a method needs to be protected and then integrations will
> > > > then be
> > > > > > > > able to hack inside the DefaultInstanceManager. You can see
> > this
> > > > here
> > > > > > > > in the OWB integration:
> > > > > > > >
> > > > > > > >
> > > > > >
> > > >
> > https://github.com/apache/tomcat/blob/main/modules/owb/src/main/java/org/apache/webbeans/web/tomcat/OpenWebBeansInstanceManager.java#L102
> > > > > > > > (basically, first call newInstance, because there's no other
> > way,
> > > > then
> > > > > > > > inject, but newInstance will have already invoked
> > @PostConstruct).
> > > > To
> > > > > > > > fix this, I plan to add an Injector interface to
> > > > > > > > DefaultInstanceManager since this is pretty much the only way
> > to do
> > > > > > > > this cleanly in Tomcat standalone (taking over the whole
> > > > > > > > DefaultInstanceManager is clearly not the best idea).
> > > > > > > >
> > > > > > >
> > > > > > > Yeah, TomEE ([1]) had some hard time with JSP due to that and its
> > > > > > > re-implementation of the instantiation.
> > > > > > > Basically the idea was to implement it aside Tomcat default impl
> > but
> > > > for
> > > > > > > some classes it was not convenient enough.
> > > > > > > That said I have to admit I'm not sure it needs a new concept
> > > > (injector)
> > > > > > > because basically it will copy the instance manager API (inject,
> > > > release
> > > > > > or
> > > > > > > something like that since it is not only about the inject phase
> > so
> > > > either
> > > > > > > you define the lifecycle or you store a Runnable you call at
> > release
> > > > time
> > > > > > > too - isnt the abstraction too complex then?).
> > > > > >
> > > > > > For EE or some other similar embedded, the assumption was that
> > > > > > InstanceManager would be fully reimplemented. But this is more work
> > > > > > for a lighter weight integration, ok.
> > > > > >
> > > > > > The change to add the API to DefaultInstanceManager would be:
> > > > > > --- a/java/org/apache/catalina/core/DefaultInstanceManager.java
> > > > > > +++ b/java/org/apache/catalina/core/DefaultInstanceManager.java
> > > > > > @@ -109,6 +109,7 @@ public class DefaultInstanceManager implements
> > > > > > InstanceManager {
> > > > > > new ManagedConcurrentWeakHashMap<>();
> > > > > > private final Map<String, String> postConstructMethods;
> > > > > > private final Map<String, String> preDestroyMethods;
> > > > > > + private Injector injector = null;
> > > > > >
> > > > > > public DefaultInstanceManager(Context context,
> > > > > > Map<String, Map<String, String>> injectionMap,
> > > > > > @@ -172,6 +173,9 @@ public class DefaultInstanceManager implements
> > > > > > InstanceManager {
> > > > > > Map<String, String> injections =
> > > > > > assembleInjectionsFromClassHierarchy(clazz);
> > > > > > populateAnnotationsCache(clazz, injections);
> > > > > > processAnnotations(instance, injections);
> > > > > > + if (injector != null) {
> > > > > > + injector.inject(instance);
> > > > > > + }
> > > > > > postConstruct(instance, clazz);
> > > > > > }
> > > > > > return instance;
> > > > > > @@ -193,6 +197,9 @@ public class DefaultInstanceManager implements
> > > > > > InstanceManager {
> > > > > > @Override
> > > > > > public void destroyInstance(Object instance) throws
> > > > > > IllegalAccessException,
> > > > > > InvocationTargetException {
> > > > > > + if (injector != null) {
> > > > > > + injector.destroy(instance);
> > > > > > + }
> > > > > > if (!ignoreAnnotations) {
> > > > > > preDestroy(instance, instance.getClass());
> > > > > > }
> > > > > > @@ -828,4 +835,16 @@ public class DefaultInstanceManager implements
> > > > > > InstanceManager {
> > > > > > return loadClass(className, classLoader);
> > > > > > }
> > > > > > }
> > > > > > +
> > > > > > + public void setInjector(Injector injector) {
> > > > > > + if (injector == null) {
> > > > > > + this.injector = injector;
> > > > > > + }
> > > > > > + }
> > > > > > +
> > > > > > + public interface Injector {
> > > > > > + void inject(Object instance);
> > > > > > + void destroy(Object instance);
> > > > > > + }
> > > > > > +
> > > > > > }
> > > > > >
> > > > > > Simply allowing inject and destroy callbacks on the main
> > > > > > DefaultInstanceManager, which is what the OWBInstanceManager
> > > > > > integration does (except it cannot do it in the right order for
> > > > > > injection, which is what the user complained about).
> > > > > >
> > > > > > > Another issue there is that newInstance assumes a "new" which
> > means
> > > > you
> > > > > > can
> > > > > > > not use a CDI instance generally speaking, only a banalised
> > instance
> > > > > > which
> > > > > > > gets injections so it means you can just impl yourself a
> > > > > > > newInstance+whatever you want+postconstruct call (this is light
> > > > > > actually).
> > > > > > > This is where setting ignoreAnnotations would be quite fancy in
> > your
> > > > own
> > > > > > > instance manager and wouldnt need any new API.
> > > > > >
> > > > > > Here it clearly relies on the behavior of DefaultInstanceManager.
> > > > > > OTOH, maybe it's good to keep ignoreAnnotations as a real "ignore
> > all
> > > > > > annotations I know what I am doing", but decouple it from the
> > > > > > matadata-complete from EE.
> > > > > >
> > > > > > > That said, enabling to use a CDI instance would be way more
> > powerful
> > > > and
> > > > > > is
> > > > > > > not against the spec - it is actually not specified AFAIK.
> > > > > > > Indeed it would need a toggle on the OWBIM of Tomcat integration
> > but
> > > > it
> > > > > > > would also open way more doors in terms of usage because your
> > servlet
> > > > > > > (filter, ...) is now a CDI beans with a connection to its bus and
> > > > > > > interceptors.
> > > > > > > I know it can already be done by using a container initializer
> > which
> > > > gets
> > > > > > > beans injected and the instances directly passed to the
> > addServlet()
> > > > > > > (instead of the class) but it would also be a very valuable
> > addition
> > > > to
> > > > > > the
> > > > > > > module if the instantiation is reworked so I'm just mentionning
> > it
> > > > as an
> > > > > > > opportunity.
> > > > > > >
> > > > > > >
> > > > > > > [1]
> > > > > > >
> > > > > >
> > > >
> > https://github.com/apache/tomee/blob/main/tomee/tomee-catalina/src/main/java/org/apache/tomee/catalina/JavaeeInstanceManager.java
> > > > > >
> > > > > > Looking at the code, it seems reasonable to have a custom instance
> > > > > > manager for TomEE as it does more than simply inject. About JSP
> > > > > > specifically, I'm not sure I understand the problem but that's ok.
> > > > > >
> > > > > > Rémy
> > > > > >
> > > > > > >
> > > > > > >
> > > > > > > >
> > > > > > > > The impact of fixing these for users should be non-existent:
> > it is
> > > > > > > > really a Tomcat standalone only thing impacting users with some
> > > > very
> > > > > > > > precise EE needs. A full EE integration will simply take over
> > the
> > > > > > > > whole annotation processing and instance manager from Tomcat,
> > and
> > > > > > > > hopefully do The Right Thing already.
> > > > > > > >
> > > > > > > > Although this is not super critical, I plan to address these
> > > > issues in
> > > > > > > > the OWB integration (after adding the needed API change in
> > Tomcat's
> > > > > > > > DefaultInstanceManager).
> > > > > > > >
> > > > > > > > Comments ?
> > > > > > > >
> > > > > > > > Rémy
> > > > > > > >
> > > > > > > >
> > > > ---------------------------------------------------------------------
> > > > > > > > To unsubscribe, e-mail: dev-unsubscribe@tomcat.apache.org
> > > > > > > > For additional commands, e-mail: dev-help@tomcat.apache.org
> > > > > > > >
> > > > > > > >
> > > > > >
> > > > > >
> > ---------------------------------------------------------------------
> > > > > > To unsubscribe, e-mail: dev-unsubscribe@tomcat.apache.org
> > > > > > For additional commands, e-mail: dev-help@tomcat.apache.org
> > > > > >
> > > > > >
> > > >
> > > > ---------------------------------------------------------------------
> > > > To unsubscribe, e-mail: dev-unsubscribe@tomcat.apache.org
> > > > For additional commands, e-mail: dev-help@tomcat.apache.org
> > > >
> > > >
> >
> > ---------------------------------------------------------------------
> > To unsubscribe, e-mail: dev-unsubscribe@tomcat.apache.org
> > For additional commands, e-mail: dev-help@tomcat.apache.org
> >
> >
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@tomcat.apache.org
For additional commands, e-mail: dev-help@tomcat.apache.org
Re: CDI and injection issues
Posted by Romain Manni-Bucau <rm...@gmail.com>.
Le jeu. 24 nov. 2022 à 10:13, Rémy Maucherat <re...@apache.org> a écrit :
> On Wed, Nov 23, 2022 at 11:10 AM Romain Manni-Bucau
> <rm...@gmail.com> wrote:
> >
> > Well, it is not that simple.
> >
> > Two notes on that:
> >
> > 1. One point is the API, injector and instance manager are the exact same
> > API if you want a generic API so not sure it should be duplicated with
> > different names (or said otherwise the Injector API is not generic enough
> > to be worth a tomcat "core" change IMHO)
>
> This is indeed the same API.
> Tomcat standalone handles Jakarta Annotations here:
>
> https://github.com/apache/tomcat/blob/main/java/org/apache/catalina/core/DefaultInstanceManager.java#L174
> The owb integration is supposed to do the CDI annotations on the same
> spot. AFAIK, it works. Except that the PostConstruct will be done
> later.
> I don't want to duplicate DefaultInstanceManager since there are a lot
> of privileged actions and code in there. So adding the callback on
> line 174 does seem fine to me.
>
> > 2. the post construct is a conflict between tomcat world and cdi world
> > there but you have the same "conflict" (= concurrent world/handling) for
> > injections, i.e. tomcat handles persistence context, persistence unit,
> > webservice ref etc but CDI can also handle it so the injector is either
> > incomplete in one case (cdi without these handling) and should be
> combined
> > between tomcat and injector OR it is done twice, potentially differently.
> >
> > So at the end it looks like you don't have the choice to just own the
> > instance manager to ensure it is done properly so making it easier to
> > instantiate is likely the way to go but adding an abstraction which is
> not
> > generic is maybe not helping as much as it can look like upfront.
> >
> > Side note: meecrowave does a more cdi instantiation:
> >
> https://github.com/apache/openwebbeans-meecrowave/blob/master/meecrowave-core/src/main/java/org/apache/meecrowave/tomcat/CDIInstanceManager.java
> > and it is very few lines so not sure the injector would help much more
> for
> > a tomcat-cdi integration.
>
> Sure, but the problem is that I have to keep DefaultInstanceManager
> around, and I won't replace everything with a CDI implementation.
>
Well, in the owb module you will have to - previous impl is more about
letting cdi handling the postconstruct but lack the JNDI support to be
complete - to solve the double/conflict injection point anyway. The
*Injector* API does not solve it so not sure there is much space for this
invalid by design API :s. Did you find a way to solve it?
>
> Rémy
>
> >
> > 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. 23 nov. 2022 à 10:56, Rémy Maucherat <re...@apache.org> a écrit :
> >
> > > On Tue, Nov 22, 2022 at 6:08 PM Romain Manni-Bucau
> > > <rm...@gmail.com> wrote:
> > > >
> > > > Hmm, how is your injector different from an InstanceManager? (
> > > >
> > >
> https://github.com/apache/tomcat/blob/main/java/org/apache/tomcat/InstanceManager.java
> > > > )
> > > > Only by not having all `newInstance` flavors?
> > >
> > > The plan is to keep on using DefaultInstanceManager since OWB only
> > > needs a callback for injection (before PostConstruct) and destroy (to
> > > remove the instance from the map). Otherwise the whole InstanceManager
> > > has to be replaced (and done better than the current one since the
> > > PostConstruct calls cannot be in the right order otherwise).
> > >
> > > > Will also need the backgroundProcess (cache cleanup, same as instance
> > > > manager) and similarly the calling context (new instance params), in
> > > > particular the classloader.
> > > > This is why I said it sounds more like a single API and more hooking
> the
> > > > default instance to enable what you want or just redo it is likely
> > > simpler
> > > > _on an API standpoint_.
> > > > Guess postConstruct() method as in TomEE impl - but using tomcat in
> place
> > > > method, just exposing it - can be sufficient and keep the API clean
> if
> > > you
> > > > don't want to reimpl anything.
> > >
> > > This is only a callback for injection with DefaultInstanceManager.
> > > Implementing your own InstanceManager in TomEE is still needed and
> > > since it is not using DefaultInstanceManager this callback API is not
> > > available at all. Also it will not break existing InstanceManager
> > > implementations (even ones that extend DefaultInstanceManager), so
> > > that's another thing I had to consider.
> > >
> > > The user who got me into this is here:
> > > https://lists.apache.org/thread/1g4bpj3jnmmmxkh37qvwzoplqo65ldot
> > > This is about Weld, but after having a look the
> > > OpenWebBeansInstanceManager does the same thing. Then I realized
> > > unless the InstanceManager was fully replaced there would be no way to
> > > implement the proper ordering.
> > >
> > > > About JSP hack, it is more general but hits mainly JSP: it is about
> > > tomcat
> > > > specific JNDI injections, the workaround and wiring used elsewhere
> for
> > > > beans didn't work properly for JSP. Guess you don't have this issue
> but
> > > > something making it easier to handle can also probably be welcomed by
> > > > consumers.
> > >
> > > Ok.
> > >
> > > Rémy
> > >
> > > >
> > > >
> > > > Le mar. 22 nov. 2022 à 17:26, Rémy Maucherat <re...@apache.org> a
> écrit :
> > > >
> > > > > On Tue, Nov 22, 2022 at 4:30 PM Romain Manni-Bucau
> > > > > <rm...@gmail.com> wrote:
> > > > > >
> > > > > > Hi Rémy,
> > > > > >
> > > > > > I put a few comments inline hoping it helps
> > > > > >
> > > > > >
> > > > > > Le mar. 22 nov. 2022 à 16:11, Rémy Maucherat <re...@apache.org> a
> > > écrit :
> > > > > >
> > > > > > > Hi,
> > > > > > >
> > > > > > > Following a post on the user list, I have looked into CDI and
> > > > > > > injection processing in Tomcat standalone (or standalone
> extended
> > > by
> > > > > > > CDI) and found the following issues:
> > > > > > >
> > > > > > > a) metadata-complete is done wrong. The spec got retconned some
> > > time
> > > > > > > ago and metadata-complete only means Servlet spec defining
> > > metadata,
> > > > > > > such as @WebServlet (= the ones that require scanning all
> classes
> > > just
> > > > > > > in case). So in practice inside DefaultInstanceManager the
> > > > > > > ignoreAnnotations flag shouldn't be used at all and it should
> > > simply
> > > > > > > be removed. I am ok on only doing this in 11 ;)
> > > > > > >
> > > > > > > b) CDI is not intertwined with the DefaultInstanceManager.
> > > Basically
> > > > > > > as the user said, injections should be done *before*
> > > @PostConstruct,
> > > > > > > and it . There's a (minor) problem with the
> DefaultInstanceManager
> > > > > > > API, a method needs to be protected and then integrations will
> > > then be
> > > > > > > able to hack inside the DefaultInstanceManager. You can see
> this
> > > here
> > > > > > > in the OWB integration:
> > > > > > >
> > > > > > >
> > > > >
> > >
> https://github.com/apache/tomcat/blob/main/modules/owb/src/main/java/org/apache/webbeans/web/tomcat/OpenWebBeansInstanceManager.java#L102
> > > > > > > (basically, first call newInstance, because there's no other
> way,
> > > then
> > > > > > > inject, but newInstance will have already invoked
> @PostConstruct).
> > > To
> > > > > > > fix this, I plan to add an Injector interface to
> > > > > > > DefaultInstanceManager since this is pretty much the only way
> to do
> > > > > > > this cleanly in Tomcat standalone (taking over the whole
> > > > > > > DefaultInstanceManager is clearly not the best idea).
> > > > > > >
> > > > > >
> > > > > > Yeah, TomEE ([1]) had some hard time with JSP due to that and its
> > > > > > re-implementation of the instantiation.
> > > > > > Basically the idea was to implement it aside Tomcat default impl
> but
> > > for
> > > > > > some classes it was not convenient enough.
> > > > > > That said I have to admit I'm not sure it needs a new concept
> > > (injector)
> > > > > > because basically it will copy the instance manager API (inject,
> > > release
> > > > > or
> > > > > > something like that since it is not only about the inject phase
> so
> > > either
> > > > > > you define the lifecycle or you store a Runnable you call at
> release
> > > time
> > > > > > too - isnt the abstraction too complex then?).
> > > > >
> > > > > For EE or some other similar embedded, the assumption was that
> > > > > InstanceManager would be fully reimplemented. But this is more work
> > > > > for a lighter weight integration, ok.
> > > > >
> > > > > The change to add the API to DefaultInstanceManager would be:
> > > > > --- a/java/org/apache/catalina/core/DefaultInstanceManager.java
> > > > > +++ b/java/org/apache/catalina/core/DefaultInstanceManager.java
> > > > > @@ -109,6 +109,7 @@ public class DefaultInstanceManager implements
> > > > > InstanceManager {
> > > > > new ManagedConcurrentWeakHashMap<>();
> > > > > private final Map<String, String> postConstructMethods;
> > > > > private final Map<String, String> preDestroyMethods;
> > > > > + private Injector injector = null;
> > > > >
> > > > > public DefaultInstanceManager(Context context,
> > > > > Map<String, Map<String, String>> injectionMap,
> > > > > @@ -172,6 +173,9 @@ public class DefaultInstanceManager implements
> > > > > InstanceManager {
> > > > > Map<String, String> injections =
> > > > > assembleInjectionsFromClassHierarchy(clazz);
> > > > > populateAnnotationsCache(clazz, injections);
> > > > > processAnnotations(instance, injections);
> > > > > + if (injector != null) {
> > > > > + injector.inject(instance);
> > > > > + }
> > > > > postConstruct(instance, clazz);
> > > > > }
> > > > > return instance;
> > > > > @@ -193,6 +197,9 @@ public class DefaultInstanceManager implements
> > > > > InstanceManager {
> > > > > @Override
> > > > > public void destroyInstance(Object instance) throws
> > > > > IllegalAccessException,
> > > > > InvocationTargetException {
> > > > > + if (injector != null) {
> > > > > + injector.destroy(instance);
> > > > > + }
> > > > > if (!ignoreAnnotations) {
> > > > > preDestroy(instance, instance.getClass());
> > > > > }
> > > > > @@ -828,4 +835,16 @@ public class DefaultInstanceManager implements
> > > > > InstanceManager {
> > > > > return loadClass(className, classLoader);
> > > > > }
> > > > > }
> > > > > +
> > > > > + public void setInjector(Injector injector) {
> > > > > + if (injector == null) {
> > > > > + this.injector = injector;
> > > > > + }
> > > > > + }
> > > > > +
> > > > > + public interface Injector {
> > > > > + void inject(Object instance);
> > > > > + void destroy(Object instance);
> > > > > + }
> > > > > +
> > > > > }
> > > > >
> > > > > Simply allowing inject and destroy callbacks on the main
> > > > > DefaultInstanceManager, which is what the OWBInstanceManager
> > > > > integration does (except it cannot do it in the right order for
> > > > > injection, which is what the user complained about).
> > > > >
> > > > > > Another issue there is that newInstance assumes a "new" which
> means
> > > you
> > > > > can
> > > > > > not use a CDI instance generally speaking, only a banalised
> instance
> > > > > which
> > > > > > gets injections so it means you can just impl yourself a
> > > > > > newInstance+whatever you want+postconstruct call (this is light
> > > > > actually).
> > > > > > This is where setting ignoreAnnotations would be quite fancy in
> your
> > > own
> > > > > > instance manager and wouldnt need any new API.
> > > > >
> > > > > Here it clearly relies on the behavior of DefaultInstanceManager.
> > > > > OTOH, maybe it's good to keep ignoreAnnotations as a real "ignore
> all
> > > > > annotations I know what I am doing", but decouple it from the
> > > > > matadata-complete from EE.
> > > > >
> > > > > > That said, enabling to use a CDI instance would be way more
> powerful
> > > and
> > > > > is
> > > > > > not against the spec - it is actually not specified AFAIK.
> > > > > > Indeed it would need a toggle on the OWBIM of Tomcat integration
> but
> > > it
> > > > > > would also open way more doors in terms of usage because your
> servlet
> > > > > > (filter, ...) is now a CDI beans with a connection to its bus and
> > > > > > interceptors.
> > > > > > I know it can already be done by using a container initializer
> which
> > > gets
> > > > > > beans injected and the instances directly passed to the
> addServlet()
> > > > > > (instead of the class) but it would also be a very valuable
> addition
> > > to
> > > > > the
> > > > > > module if the instantiation is reworked so I'm just mentionning
> it
> > > as an
> > > > > > opportunity.
> > > > > >
> > > > > >
> > > > > > [1]
> > > > > >
> > > > >
> > >
> https://github.com/apache/tomee/blob/main/tomee/tomee-catalina/src/main/java/org/apache/tomee/catalina/JavaeeInstanceManager.java
> > > > >
> > > > > Looking at the code, it seems reasonable to have a custom instance
> > > > > manager for TomEE as it does more than simply inject. About JSP
> > > > > specifically, I'm not sure I understand the problem but that's ok.
> > > > >
> > > > > Rémy
> > > > >
> > > > > >
> > > > > >
> > > > > > >
> > > > > > > The impact of fixing these for users should be non-existent:
> it is
> > > > > > > really a Tomcat standalone only thing impacting users with some
> > > very
> > > > > > > precise EE needs. A full EE integration will simply take over
> the
> > > > > > > whole annotation processing and instance manager from Tomcat,
> and
> > > > > > > hopefully do The Right Thing already.
> > > > > > >
> > > > > > > Although this is not super critical, I plan to address these
> > > issues in
> > > > > > > the OWB integration (after adding the needed API change in
> Tomcat's
> > > > > > > DefaultInstanceManager).
> > > > > > >
> > > > > > > Comments ?
> > > > > > >
> > > > > > > Rémy
> > > > > > >
> > > > > > >
> > > ---------------------------------------------------------------------
> > > > > > > To unsubscribe, e-mail: dev-unsubscribe@tomcat.apache.org
> > > > > > > For additional commands, e-mail: dev-help@tomcat.apache.org
> > > > > > >
> > > > > > >
> > > > >
> > > > >
> ---------------------------------------------------------------------
> > > > > To unsubscribe, e-mail: dev-unsubscribe@tomcat.apache.org
> > > > > For additional commands, e-mail: dev-help@tomcat.apache.org
> > > > >
> > > > >
> > >
> > > ---------------------------------------------------------------------
> > > To unsubscribe, e-mail: dev-unsubscribe@tomcat.apache.org
> > > For additional commands, e-mail: dev-help@tomcat.apache.org
> > >
> > >
>
> ---------------------------------------------------------------------
> To unsubscribe, e-mail: dev-unsubscribe@tomcat.apache.org
> For additional commands, e-mail: dev-help@tomcat.apache.org
>
>
Re: CDI and injection issues
Posted by Rémy Maucherat <re...@apache.org>.
On Wed, Nov 23, 2022 at 11:10 AM Romain Manni-Bucau
<rm...@gmail.com> wrote:
>
> Well, it is not that simple.
>
> Two notes on that:
>
> 1. One point is the API, injector and instance manager are the exact same
> API if you want a generic API so not sure it should be duplicated with
> different names (or said otherwise the Injector API is not generic enough
> to be worth a tomcat "core" change IMHO)
This is indeed the same API.
Tomcat standalone handles Jakarta Annotations here:
https://github.com/apache/tomcat/blob/main/java/org/apache/catalina/core/DefaultInstanceManager.java#L174
The owb integration is supposed to do the CDI annotations on the same
spot. AFAIK, it works. Except that the PostConstruct will be done
later.
I don't want to duplicate DefaultInstanceManager since there are a lot
of privileged actions and code in there. So adding the callback on
line 174 does seem fine to me.
> 2. the post construct is a conflict between tomcat world and cdi world
> there but you have the same "conflict" (= concurrent world/handling) for
> injections, i.e. tomcat handles persistence context, persistence unit,
> webservice ref etc but CDI can also handle it so the injector is either
> incomplete in one case (cdi without these handling) and should be combined
> between tomcat and injector OR it is done twice, potentially differently.
>
> So at the end it looks like you don't have the choice to just own the
> instance manager to ensure it is done properly so making it easier to
> instantiate is likely the way to go but adding an abstraction which is not
> generic is maybe not helping as much as it can look like upfront.
>
> Side note: meecrowave does a more cdi instantiation:
> https://github.com/apache/openwebbeans-meecrowave/blob/master/meecrowave-core/src/main/java/org/apache/meecrowave/tomcat/CDIInstanceManager.java
> and it is very few lines so not sure the injector would help much more for
> a tomcat-cdi integration.
Sure, but the problem is that I have to keep DefaultInstanceManager
around, and I won't replace everything with a CDI implementation.
Rémy
>
> 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. 23 nov. 2022 à 10:56, Rémy Maucherat <re...@apache.org> a écrit :
>
> > On Tue, Nov 22, 2022 at 6:08 PM Romain Manni-Bucau
> > <rm...@gmail.com> wrote:
> > >
> > > Hmm, how is your injector different from an InstanceManager? (
> > >
> > https://github.com/apache/tomcat/blob/main/java/org/apache/tomcat/InstanceManager.java
> > > )
> > > Only by not having all `newInstance` flavors?
> >
> > The plan is to keep on using DefaultInstanceManager since OWB only
> > needs a callback for injection (before PostConstruct) and destroy (to
> > remove the instance from the map). Otherwise the whole InstanceManager
> > has to be replaced (and done better than the current one since the
> > PostConstruct calls cannot be in the right order otherwise).
> >
> > > Will also need the backgroundProcess (cache cleanup, same as instance
> > > manager) and similarly the calling context (new instance params), in
> > > particular the classloader.
> > > This is why I said it sounds more like a single API and more hooking the
> > > default instance to enable what you want or just redo it is likely
> > simpler
> > > _on an API standpoint_.
> > > Guess postConstruct() method as in TomEE impl - but using tomcat in place
> > > method, just exposing it - can be sufficient and keep the API clean if
> > you
> > > don't want to reimpl anything.
> >
> > This is only a callback for injection with DefaultInstanceManager.
> > Implementing your own InstanceManager in TomEE is still needed and
> > since it is not using DefaultInstanceManager this callback API is not
> > available at all. Also it will not break existing InstanceManager
> > implementations (even ones that extend DefaultInstanceManager), so
> > that's another thing I had to consider.
> >
> > The user who got me into this is here:
> > https://lists.apache.org/thread/1g4bpj3jnmmmxkh37qvwzoplqo65ldot
> > This is about Weld, but after having a look the
> > OpenWebBeansInstanceManager does the same thing. Then I realized
> > unless the InstanceManager was fully replaced there would be no way to
> > implement the proper ordering.
> >
> > > About JSP hack, it is more general but hits mainly JSP: it is about
> > tomcat
> > > specific JNDI injections, the workaround and wiring used elsewhere for
> > > beans didn't work properly for JSP. Guess you don't have this issue but
> > > something making it easier to handle can also probably be welcomed by
> > > consumers.
> >
> > Ok.
> >
> > Rémy
> >
> > >
> > >
> > > Le mar. 22 nov. 2022 à 17:26, Rémy Maucherat <re...@apache.org> a écrit :
> > >
> > > > On Tue, Nov 22, 2022 at 4:30 PM Romain Manni-Bucau
> > > > <rm...@gmail.com> wrote:
> > > > >
> > > > > Hi Rémy,
> > > > >
> > > > > I put a few comments inline hoping it helps
> > > > >
> > > > >
> > > > > Le mar. 22 nov. 2022 à 16:11, Rémy Maucherat <re...@apache.org> a
> > écrit :
> > > > >
> > > > > > Hi,
> > > > > >
> > > > > > Following a post on the user list, I have looked into CDI and
> > > > > > injection processing in Tomcat standalone (or standalone extended
> > by
> > > > > > CDI) and found the following issues:
> > > > > >
> > > > > > a) metadata-complete is done wrong. The spec got retconned some
> > time
> > > > > > ago and metadata-complete only means Servlet spec defining
> > metadata,
> > > > > > such as @WebServlet (= the ones that require scanning all classes
> > just
> > > > > > in case). So in practice inside DefaultInstanceManager the
> > > > > > ignoreAnnotations flag shouldn't be used at all and it should
> > simply
> > > > > > be removed. I am ok on only doing this in 11 ;)
> > > > > >
> > > > > > b) CDI is not intertwined with the DefaultInstanceManager.
> > Basically
> > > > > > as the user said, injections should be done *before*
> > @PostConstruct,
> > > > > > and it . There's a (minor) problem with the DefaultInstanceManager
> > > > > > API, a method needs to be protected and then integrations will
> > then be
> > > > > > able to hack inside the DefaultInstanceManager. You can see this
> > here
> > > > > > in the OWB integration:
> > > > > >
> > > > > >
> > > >
> > https://github.com/apache/tomcat/blob/main/modules/owb/src/main/java/org/apache/webbeans/web/tomcat/OpenWebBeansInstanceManager.java#L102
> > > > > > (basically, first call newInstance, because there's no other way,
> > then
> > > > > > inject, but newInstance will have already invoked @PostConstruct).
> > To
> > > > > > fix this, I plan to add an Injector interface to
> > > > > > DefaultInstanceManager since this is pretty much the only way to do
> > > > > > this cleanly in Tomcat standalone (taking over the whole
> > > > > > DefaultInstanceManager is clearly not the best idea).
> > > > > >
> > > > >
> > > > > Yeah, TomEE ([1]) had some hard time with JSP due to that and its
> > > > > re-implementation of the instantiation.
> > > > > Basically the idea was to implement it aside Tomcat default impl but
> > for
> > > > > some classes it was not convenient enough.
> > > > > That said I have to admit I'm not sure it needs a new concept
> > (injector)
> > > > > because basically it will copy the instance manager API (inject,
> > release
> > > > or
> > > > > something like that since it is not only about the inject phase so
> > either
> > > > > you define the lifecycle or you store a Runnable you call at release
> > time
> > > > > too - isnt the abstraction too complex then?).
> > > >
> > > > For EE or some other similar embedded, the assumption was that
> > > > InstanceManager would be fully reimplemented. But this is more work
> > > > for a lighter weight integration, ok.
> > > >
> > > > The change to add the API to DefaultInstanceManager would be:
> > > > --- a/java/org/apache/catalina/core/DefaultInstanceManager.java
> > > > +++ b/java/org/apache/catalina/core/DefaultInstanceManager.java
> > > > @@ -109,6 +109,7 @@ public class DefaultInstanceManager implements
> > > > InstanceManager {
> > > > new ManagedConcurrentWeakHashMap<>();
> > > > private final Map<String, String> postConstructMethods;
> > > > private final Map<String, String> preDestroyMethods;
> > > > + private Injector injector = null;
> > > >
> > > > public DefaultInstanceManager(Context context,
> > > > Map<String, Map<String, String>> injectionMap,
> > > > @@ -172,6 +173,9 @@ public class DefaultInstanceManager implements
> > > > InstanceManager {
> > > > Map<String, String> injections =
> > > > assembleInjectionsFromClassHierarchy(clazz);
> > > > populateAnnotationsCache(clazz, injections);
> > > > processAnnotations(instance, injections);
> > > > + if (injector != null) {
> > > > + injector.inject(instance);
> > > > + }
> > > > postConstruct(instance, clazz);
> > > > }
> > > > return instance;
> > > > @@ -193,6 +197,9 @@ public class DefaultInstanceManager implements
> > > > InstanceManager {
> > > > @Override
> > > > public void destroyInstance(Object instance) throws
> > > > IllegalAccessException,
> > > > InvocationTargetException {
> > > > + if (injector != null) {
> > > > + injector.destroy(instance);
> > > > + }
> > > > if (!ignoreAnnotations) {
> > > > preDestroy(instance, instance.getClass());
> > > > }
> > > > @@ -828,4 +835,16 @@ public class DefaultInstanceManager implements
> > > > InstanceManager {
> > > > return loadClass(className, classLoader);
> > > > }
> > > > }
> > > > +
> > > > + public void setInjector(Injector injector) {
> > > > + if (injector == null) {
> > > > + this.injector = injector;
> > > > + }
> > > > + }
> > > > +
> > > > + public interface Injector {
> > > > + void inject(Object instance);
> > > > + void destroy(Object instance);
> > > > + }
> > > > +
> > > > }
> > > >
> > > > Simply allowing inject and destroy callbacks on the main
> > > > DefaultInstanceManager, which is what the OWBInstanceManager
> > > > integration does (except it cannot do it in the right order for
> > > > injection, which is what the user complained about).
> > > >
> > > > > Another issue there is that newInstance assumes a "new" which means
> > you
> > > > can
> > > > > not use a CDI instance generally speaking, only a banalised instance
> > > > which
> > > > > gets injections so it means you can just impl yourself a
> > > > > newInstance+whatever you want+postconstruct call (this is light
> > > > actually).
> > > > > This is where setting ignoreAnnotations would be quite fancy in your
> > own
> > > > > instance manager and wouldnt need any new API.
> > > >
> > > > Here it clearly relies on the behavior of DefaultInstanceManager.
> > > > OTOH, maybe it's good to keep ignoreAnnotations as a real "ignore all
> > > > annotations I know what I am doing", but decouple it from the
> > > > matadata-complete from EE.
> > > >
> > > > > That said, enabling to use a CDI instance would be way more powerful
> > and
> > > > is
> > > > > not against the spec - it is actually not specified AFAIK.
> > > > > Indeed it would need a toggle on the OWBIM of Tomcat integration but
> > it
> > > > > would also open way more doors in terms of usage because your servlet
> > > > > (filter, ...) is now a CDI beans with a connection to its bus and
> > > > > interceptors.
> > > > > I know it can already be done by using a container initializer which
> > gets
> > > > > beans injected and the instances directly passed to the addServlet()
> > > > > (instead of the class) but it would also be a very valuable addition
> > to
> > > > the
> > > > > module if the instantiation is reworked so I'm just mentionning it
> > as an
> > > > > opportunity.
> > > > >
> > > > >
> > > > > [1]
> > > > >
> > > >
> > https://github.com/apache/tomee/blob/main/tomee/tomee-catalina/src/main/java/org/apache/tomee/catalina/JavaeeInstanceManager.java
> > > >
> > > > Looking at the code, it seems reasonable to have a custom instance
> > > > manager for TomEE as it does more than simply inject. About JSP
> > > > specifically, I'm not sure I understand the problem but that's ok.
> > > >
> > > > Rémy
> > > >
> > > > >
> > > > >
> > > > > >
> > > > > > The impact of fixing these for users should be non-existent: it is
> > > > > > really a Tomcat standalone only thing impacting users with some
> > very
> > > > > > precise EE needs. A full EE integration will simply take over the
> > > > > > whole annotation processing and instance manager from Tomcat, and
> > > > > > hopefully do The Right Thing already.
> > > > > >
> > > > > > Although this is not super critical, I plan to address these
> > issues in
> > > > > > the OWB integration (after adding the needed API change in Tomcat's
> > > > > > DefaultInstanceManager).
> > > > > >
> > > > > > Comments ?
> > > > > >
> > > > > > Rémy
> > > > > >
> > > > > >
> > ---------------------------------------------------------------------
> > > > > > To unsubscribe, e-mail: dev-unsubscribe@tomcat.apache.org
> > > > > > For additional commands, e-mail: dev-help@tomcat.apache.org
> > > > > >
> > > > > >
> > > >
> > > > ---------------------------------------------------------------------
> > > > To unsubscribe, e-mail: dev-unsubscribe@tomcat.apache.org
> > > > For additional commands, e-mail: dev-help@tomcat.apache.org
> > > >
> > > >
> >
> > ---------------------------------------------------------------------
> > To unsubscribe, e-mail: dev-unsubscribe@tomcat.apache.org
> > For additional commands, e-mail: dev-help@tomcat.apache.org
> >
> >
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@tomcat.apache.org
For additional commands, e-mail: dev-help@tomcat.apache.org
Re: CDI and injection issues
Posted by Romain Manni-Bucau <rm...@gmail.com>.
Well, it is not that simple.
Two notes on that:
1. One point is the API, injector and instance manager are the exact same
API if you want a generic API so not sure it should be duplicated with
different names (or said otherwise the Injector API is not generic enough
to be worth a tomcat "core" change IMHO)
2. the post construct is a conflict between tomcat world and cdi world
there but you have the same "conflict" (= concurrent world/handling) for
injections, i.e. tomcat handles persistence context, persistence unit,
webservice ref etc but CDI can also handle it so the injector is either
incomplete in one case (cdi without these handling) and should be combined
between tomcat and injector OR it is done twice, potentially differently.
So at the end it looks like you don't have the choice to just own the
instance manager to ensure it is done properly so making it easier to
instantiate is likely the way to go but adding an abstraction which is not
generic is maybe not helping as much as it can look like upfront.
Side note: meecrowave does a more cdi instantiation:
https://github.com/apache/openwebbeans-meecrowave/blob/master/meecrowave-core/src/main/java/org/apache/meecrowave/tomcat/CDIInstanceManager.java
and it is very few lines so not sure the injector would help much more for
a tomcat-cdi integration.
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. 23 nov. 2022 à 10:56, Rémy Maucherat <re...@apache.org> a écrit :
> On Tue, Nov 22, 2022 at 6:08 PM Romain Manni-Bucau
> <rm...@gmail.com> wrote:
> >
> > Hmm, how is your injector different from an InstanceManager? (
> >
> https://github.com/apache/tomcat/blob/main/java/org/apache/tomcat/InstanceManager.java
> > )
> > Only by not having all `newInstance` flavors?
>
> The plan is to keep on using DefaultInstanceManager since OWB only
> needs a callback for injection (before PostConstruct) and destroy (to
> remove the instance from the map). Otherwise the whole InstanceManager
> has to be replaced (and done better than the current one since the
> PostConstruct calls cannot be in the right order otherwise).
>
> > Will also need the backgroundProcess (cache cleanup, same as instance
> > manager) and similarly the calling context (new instance params), in
> > particular the classloader.
> > This is why I said it sounds more like a single API and more hooking the
> > default instance to enable what you want or just redo it is likely
> simpler
> > _on an API standpoint_.
> > Guess postConstruct() method as in TomEE impl - but using tomcat in place
> > method, just exposing it - can be sufficient and keep the API clean if
> you
> > don't want to reimpl anything.
>
> This is only a callback for injection with DefaultInstanceManager.
> Implementing your own InstanceManager in TomEE is still needed and
> since it is not using DefaultInstanceManager this callback API is not
> available at all. Also it will not break existing InstanceManager
> implementations (even ones that extend DefaultInstanceManager), so
> that's another thing I had to consider.
>
> The user who got me into this is here:
> https://lists.apache.org/thread/1g4bpj3jnmmmxkh37qvwzoplqo65ldot
> This is about Weld, but after having a look the
> OpenWebBeansInstanceManager does the same thing. Then I realized
> unless the InstanceManager was fully replaced there would be no way to
> implement the proper ordering.
>
> > About JSP hack, it is more general but hits mainly JSP: it is about
> tomcat
> > specific JNDI injections, the workaround and wiring used elsewhere for
> > beans didn't work properly for JSP. Guess you don't have this issue but
> > something making it easier to handle can also probably be welcomed by
> > consumers.
>
> Ok.
>
> Rémy
>
> >
> >
> > Le mar. 22 nov. 2022 à 17:26, Rémy Maucherat <re...@apache.org> a écrit :
> >
> > > On Tue, Nov 22, 2022 at 4:30 PM Romain Manni-Bucau
> > > <rm...@gmail.com> wrote:
> > > >
> > > > Hi Rémy,
> > > >
> > > > I put a few comments inline hoping it helps
> > > >
> > > >
> > > > Le mar. 22 nov. 2022 à 16:11, Rémy Maucherat <re...@apache.org> a
> écrit :
> > > >
> > > > > Hi,
> > > > >
> > > > > Following a post on the user list, I have looked into CDI and
> > > > > injection processing in Tomcat standalone (or standalone extended
> by
> > > > > CDI) and found the following issues:
> > > > >
> > > > > a) metadata-complete is done wrong. The spec got retconned some
> time
> > > > > ago and metadata-complete only means Servlet spec defining
> metadata,
> > > > > such as @WebServlet (= the ones that require scanning all classes
> just
> > > > > in case). So in practice inside DefaultInstanceManager the
> > > > > ignoreAnnotations flag shouldn't be used at all and it should
> simply
> > > > > be removed. I am ok on only doing this in 11 ;)
> > > > >
> > > > > b) CDI is not intertwined with the DefaultInstanceManager.
> Basically
> > > > > as the user said, injections should be done *before*
> @PostConstruct,
> > > > > and it . There's a (minor) problem with the DefaultInstanceManager
> > > > > API, a method needs to be protected and then integrations will
> then be
> > > > > able to hack inside the DefaultInstanceManager. You can see this
> here
> > > > > in the OWB integration:
> > > > >
> > > > >
> > >
> https://github.com/apache/tomcat/blob/main/modules/owb/src/main/java/org/apache/webbeans/web/tomcat/OpenWebBeansInstanceManager.java#L102
> > > > > (basically, first call newInstance, because there's no other way,
> then
> > > > > inject, but newInstance will have already invoked @PostConstruct).
> To
> > > > > fix this, I plan to add an Injector interface to
> > > > > DefaultInstanceManager since this is pretty much the only way to do
> > > > > this cleanly in Tomcat standalone (taking over the whole
> > > > > DefaultInstanceManager is clearly not the best idea).
> > > > >
> > > >
> > > > Yeah, TomEE ([1]) had some hard time with JSP due to that and its
> > > > re-implementation of the instantiation.
> > > > Basically the idea was to implement it aside Tomcat default impl but
> for
> > > > some classes it was not convenient enough.
> > > > That said I have to admit I'm not sure it needs a new concept
> (injector)
> > > > because basically it will copy the instance manager API (inject,
> release
> > > or
> > > > something like that since it is not only about the inject phase so
> either
> > > > you define the lifecycle or you store a Runnable you call at release
> time
> > > > too - isnt the abstraction too complex then?).
> > >
> > > For EE or some other similar embedded, the assumption was that
> > > InstanceManager would be fully reimplemented. But this is more work
> > > for a lighter weight integration, ok.
> > >
> > > The change to add the API to DefaultInstanceManager would be:
> > > --- a/java/org/apache/catalina/core/DefaultInstanceManager.java
> > > +++ b/java/org/apache/catalina/core/DefaultInstanceManager.java
> > > @@ -109,6 +109,7 @@ public class DefaultInstanceManager implements
> > > InstanceManager {
> > > new ManagedConcurrentWeakHashMap<>();
> > > private final Map<String, String> postConstructMethods;
> > > private final Map<String, String> preDestroyMethods;
> > > + private Injector injector = null;
> > >
> > > public DefaultInstanceManager(Context context,
> > > Map<String, Map<String, String>> injectionMap,
> > > @@ -172,6 +173,9 @@ public class DefaultInstanceManager implements
> > > InstanceManager {
> > > Map<String, String> injections =
> > > assembleInjectionsFromClassHierarchy(clazz);
> > > populateAnnotationsCache(clazz, injections);
> > > processAnnotations(instance, injections);
> > > + if (injector != null) {
> > > + injector.inject(instance);
> > > + }
> > > postConstruct(instance, clazz);
> > > }
> > > return instance;
> > > @@ -193,6 +197,9 @@ public class DefaultInstanceManager implements
> > > InstanceManager {
> > > @Override
> > > public void destroyInstance(Object instance) throws
> > > IllegalAccessException,
> > > InvocationTargetException {
> > > + if (injector != null) {
> > > + injector.destroy(instance);
> > > + }
> > > if (!ignoreAnnotations) {
> > > preDestroy(instance, instance.getClass());
> > > }
> > > @@ -828,4 +835,16 @@ public class DefaultInstanceManager implements
> > > InstanceManager {
> > > return loadClass(className, classLoader);
> > > }
> > > }
> > > +
> > > + public void setInjector(Injector injector) {
> > > + if (injector == null) {
> > > + this.injector = injector;
> > > + }
> > > + }
> > > +
> > > + public interface Injector {
> > > + void inject(Object instance);
> > > + void destroy(Object instance);
> > > + }
> > > +
> > > }
> > >
> > > Simply allowing inject and destroy callbacks on the main
> > > DefaultInstanceManager, which is what the OWBInstanceManager
> > > integration does (except it cannot do it in the right order for
> > > injection, which is what the user complained about).
> > >
> > > > Another issue there is that newInstance assumes a "new" which means
> you
> > > can
> > > > not use a CDI instance generally speaking, only a banalised instance
> > > which
> > > > gets injections so it means you can just impl yourself a
> > > > newInstance+whatever you want+postconstruct call (this is light
> > > actually).
> > > > This is where setting ignoreAnnotations would be quite fancy in your
> own
> > > > instance manager and wouldnt need any new API.
> > >
> > > Here it clearly relies on the behavior of DefaultInstanceManager.
> > > OTOH, maybe it's good to keep ignoreAnnotations as a real "ignore all
> > > annotations I know what I am doing", but decouple it from the
> > > matadata-complete from EE.
> > >
> > > > That said, enabling to use a CDI instance would be way more powerful
> and
> > > is
> > > > not against the spec - it is actually not specified AFAIK.
> > > > Indeed it would need a toggle on the OWBIM of Tomcat integration but
> it
> > > > would also open way more doors in terms of usage because your servlet
> > > > (filter, ...) is now a CDI beans with a connection to its bus and
> > > > interceptors.
> > > > I know it can already be done by using a container initializer which
> gets
> > > > beans injected and the instances directly passed to the addServlet()
> > > > (instead of the class) but it would also be a very valuable addition
> to
> > > the
> > > > module if the instantiation is reworked so I'm just mentionning it
> as an
> > > > opportunity.
> > > >
> > > >
> > > > [1]
> > > >
> > >
> https://github.com/apache/tomee/blob/main/tomee/tomee-catalina/src/main/java/org/apache/tomee/catalina/JavaeeInstanceManager.java
> > >
> > > Looking at the code, it seems reasonable to have a custom instance
> > > manager for TomEE as it does more than simply inject. About JSP
> > > specifically, I'm not sure I understand the problem but that's ok.
> > >
> > > Rémy
> > >
> > > >
> > > >
> > > > >
> > > > > The impact of fixing these for users should be non-existent: it is
> > > > > really a Tomcat standalone only thing impacting users with some
> very
> > > > > precise EE needs. A full EE integration will simply take over the
> > > > > whole annotation processing and instance manager from Tomcat, and
> > > > > hopefully do The Right Thing already.
> > > > >
> > > > > Although this is not super critical, I plan to address these
> issues in
> > > > > the OWB integration (after adding the needed API change in Tomcat's
> > > > > DefaultInstanceManager).
> > > > >
> > > > > Comments ?
> > > > >
> > > > > Rémy
> > > > >
> > > > >
> ---------------------------------------------------------------------
> > > > > To unsubscribe, e-mail: dev-unsubscribe@tomcat.apache.org
> > > > > For additional commands, e-mail: dev-help@tomcat.apache.org
> > > > >
> > > > >
> > >
> > > ---------------------------------------------------------------------
> > > To unsubscribe, e-mail: dev-unsubscribe@tomcat.apache.org
> > > For additional commands, e-mail: dev-help@tomcat.apache.org
> > >
> > >
>
> ---------------------------------------------------------------------
> To unsubscribe, e-mail: dev-unsubscribe@tomcat.apache.org
> For additional commands, e-mail: dev-help@tomcat.apache.org
>
>
Re: CDI and injection issues
Posted by Rémy Maucherat <re...@apache.org>.
On Tue, Nov 22, 2022 at 6:08 PM Romain Manni-Bucau
<rm...@gmail.com> wrote:
>
> Hmm, how is your injector different from an InstanceManager? (
> https://github.com/apache/tomcat/blob/main/java/org/apache/tomcat/InstanceManager.java
> )
> Only by not having all `newInstance` flavors?
The plan is to keep on using DefaultInstanceManager since OWB only
needs a callback for injection (before PostConstruct) and destroy (to
remove the instance from the map). Otherwise the whole InstanceManager
has to be replaced (and done better than the current one since the
PostConstruct calls cannot be in the right order otherwise).
> Will also need the backgroundProcess (cache cleanup, same as instance
> manager) and similarly the calling context (new instance params), in
> particular the classloader.
> This is why I said it sounds more like a single API and more hooking the
> default instance to enable what you want or just redo it is likely simpler
> _on an API standpoint_.
> Guess postConstruct() method as in TomEE impl - but using tomcat in place
> method, just exposing it - can be sufficient and keep the API clean if you
> don't want to reimpl anything.
This is only a callback for injection with DefaultInstanceManager.
Implementing your own InstanceManager in TomEE is still needed and
since it is not using DefaultInstanceManager this callback API is not
available at all. Also it will not break existing InstanceManager
implementations (even ones that extend DefaultInstanceManager), so
that's another thing I had to consider.
The user who got me into this is here:
https://lists.apache.org/thread/1g4bpj3jnmmmxkh37qvwzoplqo65ldot
This is about Weld, but after having a look the
OpenWebBeansInstanceManager does the same thing. Then I realized
unless the InstanceManager was fully replaced there would be no way to
implement the proper ordering.
> About JSP hack, it is more general but hits mainly JSP: it is about tomcat
> specific JNDI injections, the workaround and wiring used elsewhere for
> beans didn't work properly for JSP. Guess you don't have this issue but
> something making it easier to handle can also probably be welcomed by
> consumers.
Ok.
Rémy
>
>
> Le mar. 22 nov. 2022 à 17:26, Rémy Maucherat <re...@apache.org> a écrit :
>
> > On Tue, Nov 22, 2022 at 4:30 PM Romain Manni-Bucau
> > <rm...@gmail.com> wrote:
> > >
> > > Hi Rémy,
> > >
> > > I put a few comments inline hoping it helps
> > >
> > >
> > > Le mar. 22 nov. 2022 à 16:11, Rémy Maucherat <re...@apache.org> a écrit :
> > >
> > > > Hi,
> > > >
> > > > Following a post on the user list, I have looked into CDI and
> > > > injection processing in Tomcat standalone (or standalone extended by
> > > > CDI) and found the following issues:
> > > >
> > > > a) metadata-complete is done wrong. The spec got retconned some time
> > > > ago and metadata-complete only means Servlet spec defining metadata,
> > > > such as @WebServlet (= the ones that require scanning all classes just
> > > > in case). So in practice inside DefaultInstanceManager the
> > > > ignoreAnnotations flag shouldn't be used at all and it should simply
> > > > be removed. I am ok on only doing this in 11 ;)
> > > >
> > > > b) CDI is not intertwined with the DefaultInstanceManager. Basically
> > > > as the user said, injections should be done *before* @PostConstruct,
> > > > and it . There's a (minor) problem with the DefaultInstanceManager
> > > > API, a method needs to be protected and then integrations will then be
> > > > able to hack inside the DefaultInstanceManager. You can see this here
> > > > in the OWB integration:
> > > >
> > > >
> > https://github.com/apache/tomcat/blob/main/modules/owb/src/main/java/org/apache/webbeans/web/tomcat/OpenWebBeansInstanceManager.java#L102
> > > > (basically, first call newInstance, because there's no other way, then
> > > > inject, but newInstance will have already invoked @PostConstruct). To
> > > > fix this, I plan to add an Injector interface to
> > > > DefaultInstanceManager since this is pretty much the only way to do
> > > > this cleanly in Tomcat standalone (taking over the whole
> > > > DefaultInstanceManager is clearly not the best idea).
> > > >
> > >
> > > Yeah, TomEE ([1]) had some hard time with JSP due to that and its
> > > re-implementation of the instantiation.
> > > Basically the idea was to implement it aside Tomcat default impl but for
> > > some classes it was not convenient enough.
> > > That said I have to admit I'm not sure it needs a new concept (injector)
> > > because basically it will copy the instance manager API (inject, release
> > or
> > > something like that since it is not only about the inject phase so either
> > > you define the lifecycle or you store a Runnable you call at release time
> > > too - isnt the abstraction too complex then?).
> >
> > For EE or some other similar embedded, the assumption was that
> > InstanceManager would be fully reimplemented. But this is more work
> > for a lighter weight integration, ok.
> >
> > The change to add the API to DefaultInstanceManager would be:
> > --- a/java/org/apache/catalina/core/DefaultInstanceManager.java
> > +++ b/java/org/apache/catalina/core/DefaultInstanceManager.java
> > @@ -109,6 +109,7 @@ public class DefaultInstanceManager implements
> > InstanceManager {
> > new ManagedConcurrentWeakHashMap<>();
> > private final Map<String, String> postConstructMethods;
> > private final Map<String, String> preDestroyMethods;
> > + private Injector injector = null;
> >
> > public DefaultInstanceManager(Context context,
> > Map<String, Map<String, String>> injectionMap,
> > @@ -172,6 +173,9 @@ public class DefaultInstanceManager implements
> > InstanceManager {
> > Map<String, String> injections =
> > assembleInjectionsFromClassHierarchy(clazz);
> > populateAnnotationsCache(clazz, injections);
> > processAnnotations(instance, injections);
> > + if (injector != null) {
> > + injector.inject(instance);
> > + }
> > postConstruct(instance, clazz);
> > }
> > return instance;
> > @@ -193,6 +197,9 @@ public class DefaultInstanceManager implements
> > InstanceManager {
> > @Override
> > public void destroyInstance(Object instance) throws
> > IllegalAccessException,
> > InvocationTargetException {
> > + if (injector != null) {
> > + injector.destroy(instance);
> > + }
> > if (!ignoreAnnotations) {
> > preDestroy(instance, instance.getClass());
> > }
> > @@ -828,4 +835,16 @@ public class DefaultInstanceManager implements
> > InstanceManager {
> > return loadClass(className, classLoader);
> > }
> > }
> > +
> > + public void setInjector(Injector injector) {
> > + if (injector == null) {
> > + this.injector = injector;
> > + }
> > + }
> > +
> > + public interface Injector {
> > + void inject(Object instance);
> > + void destroy(Object instance);
> > + }
> > +
> > }
> >
> > Simply allowing inject and destroy callbacks on the main
> > DefaultInstanceManager, which is what the OWBInstanceManager
> > integration does (except it cannot do it in the right order for
> > injection, which is what the user complained about).
> >
> > > Another issue there is that newInstance assumes a "new" which means you
> > can
> > > not use a CDI instance generally speaking, only a banalised instance
> > which
> > > gets injections so it means you can just impl yourself a
> > > newInstance+whatever you want+postconstruct call (this is light
> > actually).
> > > This is where setting ignoreAnnotations would be quite fancy in your own
> > > instance manager and wouldnt need any new API.
> >
> > Here it clearly relies on the behavior of DefaultInstanceManager.
> > OTOH, maybe it's good to keep ignoreAnnotations as a real "ignore all
> > annotations I know what I am doing", but decouple it from the
> > matadata-complete from EE.
> >
> > > That said, enabling to use a CDI instance would be way more powerful and
> > is
> > > not against the spec - it is actually not specified AFAIK.
> > > Indeed it would need a toggle on the OWBIM of Tomcat integration but it
> > > would also open way more doors in terms of usage because your servlet
> > > (filter, ...) is now a CDI beans with a connection to its bus and
> > > interceptors.
> > > I know it can already be done by using a container initializer which gets
> > > beans injected and the instances directly passed to the addServlet()
> > > (instead of the class) but it would also be a very valuable addition to
> > the
> > > module if the instantiation is reworked so I'm just mentionning it as an
> > > opportunity.
> > >
> > >
> > > [1]
> > >
> > https://github.com/apache/tomee/blob/main/tomee/tomee-catalina/src/main/java/org/apache/tomee/catalina/JavaeeInstanceManager.java
> >
> > Looking at the code, it seems reasonable to have a custom instance
> > manager for TomEE as it does more than simply inject. About JSP
> > specifically, I'm not sure I understand the problem but that's ok.
> >
> > Rémy
> >
> > >
> > >
> > > >
> > > > The impact of fixing these for users should be non-existent: it is
> > > > really a Tomcat standalone only thing impacting users with some very
> > > > precise EE needs. A full EE integration will simply take over the
> > > > whole annotation processing and instance manager from Tomcat, and
> > > > hopefully do The Right Thing already.
> > > >
> > > > Although this is not super critical, I plan to address these issues in
> > > > the OWB integration (after adding the needed API change in Tomcat's
> > > > DefaultInstanceManager).
> > > >
> > > > Comments ?
> > > >
> > > > Rémy
> > > >
> > > > ---------------------------------------------------------------------
> > > > To unsubscribe, e-mail: dev-unsubscribe@tomcat.apache.org
> > > > For additional commands, e-mail: dev-help@tomcat.apache.org
> > > >
> > > >
> >
> > ---------------------------------------------------------------------
> > To unsubscribe, e-mail: dev-unsubscribe@tomcat.apache.org
> > For additional commands, e-mail: dev-help@tomcat.apache.org
> >
> >
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@tomcat.apache.org
For additional commands, e-mail: dev-help@tomcat.apache.org
Re: CDI and injection issues
Posted by Romain Manni-Bucau <rm...@gmail.com>.
Hmm, how is your injector different from an InstanceManager? (
https://github.com/apache/tomcat/blob/main/java/org/apache/tomcat/InstanceManager.java
)
Only by not having all `newInstance` flavors?
Will also need the backgroundProcess (cache cleanup, same as instance
manager) and similarly the calling context (new instance params), in
particular the classloader.
This is why I said it sounds more like a single API and more hooking the
default instance to enable what you want or just redo it is likely simpler
_on an API standpoint_.
Guess postConstruct() method as in TomEE impl - but using tomcat in place
method, just exposing it - can be sufficient and keep the API clean if you
don't want to reimpl anything.
About JSP hack, it is more general but hits mainly JSP: it is about tomcat
specific JNDI injections, the workaround and wiring used elsewhere for
beans didn't work properly for JSP. Guess you don't have this issue but
something making it easier to handle can also probably be welcomed by
consumers.
Le mar. 22 nov. 2022 à 17:26, Rémy Maucherat <re...@apache.org> a écrit :
> On Tue, Nov 22, 2022 at 4:30 PM Romain Manni-Bucau
> <rm...@gmail.com> wrote:
> >
> > Hi Rémy,
> >
> > I put a few comments inline hoping it helps
> >
> >
> > Le mar. 22 nov. 2022 à 16:11, Rémy Maucherat <re...@apache.org> a écrit :
> >
> > > Hi,
> > >
> > > Following a post on the user list, I have looked into CDI and
> > > injection processing in Tomcat standalone (or standalone extended by
> > > CDI) and found the following issues:
> > >
> > > a) metadata-complete is done wrong. The spec got retconned some time
> > > ago and metadata-complete only means Servlet spec defining metadata,
> > > such as @WebServlet (= the ones that require scanning all classes just
> > > in case). So in practice inside DefaultInstanceManager the
> > > ignoreAnnotations flag shouldn't be used at all and it should simply
> > > be removed. I am ok on only doing this in 11 ;)
> > >
> > > b) CDI is not intertwined with the DefaultInstanceManager. Basically
> > > as the user said, injections should be done *before* @PostConstruct,
> > > and it . There's a (minor) problem with the DefaultInstanceManager
> > > API, a method needs to be protected and then integrations will then be
> > > able to hack inside the DefaultInstanceManager. You can see this here
> > > in the OWB integration:
> > >
> > >
> https://github.com/apache/tomcat/blob/main/modules/owb/src/main/java/org/apache/webbeans/web/tomcat/OpenWebBeansInstanceManager.java#L102
> > > (basically, first call newInstance, because there's no other way, then
> > > inject, but newInstance will have already invoked @PostConstruct). To
> > > fix this, I plan to add an Injector interface to
> > > DefaultInstanceManager since this is pretty much the only way to do
> > > this cleanly in Tomcat standalone (taking over the whole
> > > DefaultInstanceManager is clearly not the best idea).
> > >
> >
> > Yeah, TomEE ([1]) had some hard time with JSP due to that and its
> > re-implementation of the instantiation.
> > Basically the idea was to implement it aside Tomcat default impl but for
> > some classes it was not convenient enough.
> > That said I have to admit I'm not sure it needs a new concept (injector)
> > because basically it will copy the instance manager API (inject, release
> or
> > something like that since it is not only about the inject phase so either
> > you define the lifecycle or you store a Runnable you call at release time
> > too - isnt the abstraction too complex then?).
>
> For EE or some other similar embedded, the assumption was that
> InstanceManager would be fully reimplemented. But this is more work
> for a lighter weight integration, ok.
>
> The change to add the API to DefaultInstanceManager would be:
> --- a/java/org/apache/catalina/core/DefaultInstanceManager.java
> +++ b/java/org/apache/catalina/core/DefaultInstanceManager.java
> @@ -109,6 +109,7 @@ public class DefaultInstanceManager implements
> InstanceManager {
> new ManagedConcurrentWeakHashMap<>();
> private final Map<String, String> postConstructMethods;
> private final Map<String, String> preDestroyMethods;
> + private Injector injector = null;
>
> public DefaultInstanceManager(Context context,
> Map<String, Map<String, String>> injectionMap,
> @@ -172,6 +173,9 @@ public class DefaultInstanceManager implements
> InstanceManager {
> Map<String, String> injections =
> assembleInjectionsFromClassHierarchy(clazz);
> populateAnnotationsCache(clazz, injections);
> processAnnotations(instance, injections);
> + if (injector != null) {
> + injector.inject(instance);
> + }
> postConstruct(instance, clazz);
> }
> return instance;
> @@ -193,6 +197,9 @@ public class DefaultInstanceManager implements
> InstanceManager {
> @Override
> public void destroyInstance(Object instance) throws
> IllegalAccessException,
> InvocationTargetException {
> + if (injector != null) {
> + injector.destroy(instance);
> + }
> if (!ignoreAnnotations) {
> preDestroy(instance, instance.getClass());
> }
> @@ -828,4 +835,16 @@ public class DefaultInstanceManager implements
> InstanceManager {
> return loadClass(className, classLoader);
> }
> }
> +
> + public void setInjector(Injector injector) {
> + if (injector == null) {
> + this.injector = injector;
> + }
> + }
> +
> + public interface Injector {
> + void inject(Object instance);
> + void destroy(Object instance);
> + }
> +
> }
>
> Simply allowing inject and destroy callbacks on the main
> DefaultInstanceManager, which is what the OWBInstanceManager
> integration does (except it cannot do it in the right order for
> injection, which is what the user complained about).
>
> > Another issue there is that newInstance assumes a "new" which means you
> can
> > not use a CDI instance generally speaking, only a banalised instance
> which
> > gets injections so it means you can just impl yourself a
> > newInstance+whatever you want+postconstruct call (this is light
> actually).
> > This is where setting ignoreAnnotations would be quite fancy in your own
> > instance manager and wouldnt need any new API.
>
> Here it clearly relies on the behavior of DefaultInstanceManager.
> OTOH, maybe it's good to keep ignoreAnnotations as a real "ignore all
> annotations I know what I am doing", but decouple it from the
> matadata-complete from EE.
>
> > That said, enabling to use a CDI instance would be way more powerful and
> is
> > not against the spec - it is actually not specified AFAIK.
> > Indeed it would need a toggle on the OWBIM of Tomcat integration but it
> > would also open way more doors in terms of usage because your servlet
> > (filter, ...) is now a CDI beans with a connection to its bus and
> > interceptors.
> > I know it can already be done by using a container initializer which gets
> > beans injected and the instances directly passed to the addServlet()
> > (instead of the class) but it would also be a very valuable addition to
> the
> > module if the instantiation is reworked so I'm just mentionning it as an
> > opportunity.
> >
> >
> > [1]
> >
> https://github.com/apache/tomee/blob/main/tomee/tomee-catalina/src/main/java/org/apache/tomee/catalina/JavaeeInstanceManager.java
>
> Looking at the code, it seems reasonable to have a custom instance
> manager for TomEE as it does more than simply inject. About JSP
> specifically, I'm not sure I understand the problem but that's ok.
>
> Rémy
>
> >
> >
> > >
> > > The impact of fixing these for users should be non-existent: it is
> > > really a Tomcat standalone only thing impacting users with some very
> > > precise EE needs. A full EE integration will simply take over the
> > > whole annotation processing and instance manager from Tomcat, and
> > > hopefully do The Right Thing already.
> > >
> > > Although this is not super critical, I plan to address these issues in
> > > the OWB integration (after adding the needed API change in Tomcat's
> > > DefaultInstanceManager).
> > >
> > > Comments ?
> > >
> > > Rémy
> > >
> > > ---------------------------------------------------------------------
> > > To unsubscribe, e-mail: dev-unsubscribe@tomcat.apache.org
> > > For additional commands, e-mail: dev-help@tomcat.apache.org
> > >
> > >
>
> ---------------------------------------------------------------------
> To unsubscribe, e-mail: dev-unsubscribe@tomcat.apache.org
> For additional commands, e-mail: dev-help@tomcat.apache.org
>
>
Re: CDI and injection issues
Posted by Rémy Maucherat <re...@apache.org>.
On Tue, Nov 22, 2022 at 4:30 PM Romain Manni-Bucau
<rm...@gmail.com> wrote:
>
> Hi Rémy,
>
> I put a few comments inline hoping it helps
>
>
> Le mar. 22 nov. 2022 à 16:11, Rémy Maucherat <re...@apache.org> a écrit :
>
> > Hi,
> >
> > Following a post on the user list, I have looked into CDI and
> > injection processing in Tomcat standalone (or standalone extended by
> > CDI) and found the following issues:
> >
> > a) metadata-complete is done wrong. The spec got retconned some time
> > ago and metadata-complete only means Servlet spec defining metadata,
> > such as @WebServlet (= the ones that require scanning all classes just
> > in case). So in practice inside DefaultInstanceManager the
> > ignoreAnnotations flag shouldn't be used at all and it should simply
> > be removed. I am ok on only doing this in 11 ;)
> >
> > b) CDI is not intertwined with the DefaultInstanceManager. Basically
> > as the user said, injections should be done *before* @PostConstruct,
> > and it . There's a (minor) problem with the DefaultInstanceManager
> > API, a method needs to be protected and then integrations will then be
> > able to hack inside the DefaultInstanceManager. You can see this here
> > in the OWB integration:
> >
> > https://github.com/apache/tomcat/blob/main/modules/owb/src/main/java/org/apache/webbeans/web/tomcat/OpenWebBeansInstanceManager.java#L102
> > (basically, first call newInstance, because there's no other way, then
> > inject, but newInstance will have already invoked @PostConstruct). To
> > fix this, I plan to add an Injector interface to
> > DefaultInstanceManager since this is pretty much the only way to do
> > this cleanly in Tomcat standalone (taking over the whole
> > DefaultInstanceManager is clearly not the best idea).
> >
>
> Yeah, TomEE ([1]) had some hard time with JSP due to that and its
> re-implementation of the instantiation.
> Basically the idea was to implement it aside Tomcat default impl but for
> some classes it was not convenient enough.
> That said I have to admit I'm not sure it needs a new concept (injector)
> because basically it will copy the instance manager API (inject, release or
> something like that since it is not only about the inject phase so either
> you define the lifecycle or you store a Runnable you call at release time
> too - isnt the abstraction too complex then?).
For EE or some other similar embedded, the assumption was that
InstanceManager would be fully reimplemented. But this is more work
for a lighter weight integration, ok.
The change to add the API to DefaultInstanceManager would be:
--- a/java/org/apache/catalina/core/DefaultInstanceManager.java
+++ b/java/org/apache/catalina/core/DefaultInstanceManager.java
@@ -109,6 +109,7 @@ public class DefaultInstanceManager implements
InstanceManager {
new ManagedConcurrentWeakHashMap<>();
private final Map<String, String> postConstructMethods;
private final Map<String, String> preDestroyMethods;
+ private Injector injector = null;
public DefaultInstanceManager(Context context,
Map<String, Map<String, String>> injectionMap,
@@ -172,6 +173,9 @@ public class DefaultInstanceManager implements
InstanceManager {
Map<String, String> injections =
assembleInjectionsFromClassHierarchy(clazz);
populateAnnotationsCache(clazz, injections);
processAnnotations(instance, injections);
+ if (injector != null) {
+ injector.inject(instance);
+ }
postConstruct(instance, clazz);
}
return instance;
@@ -193,6 +197,9 @@ public class DefaultInstanceManager implements
InstanceManager {
@Override
public void destroyInstance(Object instance) throws IllegalAccessException,
InvocationTargetException {
+ if (injector != null) {
+ injector.destroy(instance);
+ }
if (!ignoreAnnotations) {
preDestroy(instance, instance.getClass());
}
@@ -828,4 +835,16 @@ public class DefaultInstanceManager implements
InstanceManager {
return loadClass(className, classLoader);
}
}
+
+ public void setInjector(Injector injector) {
+ if (injector == null) {
+ this.injector = injector;
+ }
+ }
+
+ public interface Injector {
+ void inject(Object instance);
+ void destroy(Object instance);
+ }
+
}
Simply allowing inject and destroy callbacks on the main
DefaultInstanceManager, which is what the OWBInstanceManager
integration does (except it cannot do it in the right order for
injection, which is what the user complained about).
> Another issue there is that newInstance assumes a "new" which means you can
> not use a CDI instance generally speaking, only a banalised instance which
> gets injections so it means you can just impl yourself a
> newInstance+whatever you want+postconstruct call (this is light actually).
> This is where setting ignoreAnnotations would be quite fancy in your own
> instance manager and wouldnt need any new API.
Here it clearly relies on the behavior of DefaultInstanceManager.
OTOH, maybe it's good to keep ignoreAnnotations as a real "ignore all
annotations I know what I am doing", but decouple it from the
matadata-complete from EE.
> That said, enabling to use a CDI instance would be way more powerful and is
> not against the spec - it is actually not specified AFAIK.
> Indeed it would need a toggle on the OWBIM of Tomcat integration but it
> would also open way more doors in terms of usage because your servlet
> (filter, ...) is now a CDI beans with a connection to its bus and
> interceptors.
> I know it can already be done by using a container initializer which gets
> beans injected and the instances directly passed to the addServlet()
> (instead of the class) but it would also be a very valuable addition to the
> module if the instantiation is reworked so I'm just mentionning it as an
> opportunity.
>
>
> [1]
> https://github.com/apache/tomee/blob/main/tomee/tomee-catalina/src/main/java/org/apache/tomee/catalina/JavaeeInstanceManager.java
Looking at the code, it seems reasonable to have a custom instance
manager for TomEE as it does more than simply inject. About JSP
specifically, I'm not sure I understand the problem but that's ok.
Rémy
>
>
> >
> > The impact of fixing these for users should be non-existent: it is
> > really a Tomcat standalone only thing impacting users with some very
> > precise EE needs. A full EE integration will simply take over the
> > whole annotation processing and instance manager from Tomcat, and
> > hopefully do The Right Thing already.
> >
> > Although this is not super critical, I plan to address these issues in
> > the OWB integration (after adding the needed API change in Tomcat's
> > DefaultInstanceManager).
> >
> > Comments ?
> >
> > Rémy
> >
> > ---------------------------------------------------------------------
> > To unsubscribe, e-mail: dev-unsubscribe@tomcat.apache.org
> > For additional commands, e-mail: dev-help@tomcat.apache.org
> >
> >
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@tomcat.apache.org
For additional commands, e-mail: dev-help@tomcat.apache.org
Re: CDI and injection issues
Posted by Romain Manni-Bucau <rm...@gmail.com>.
Hi Rémy,
I put a few comments inline hoping it helps
Le mar. 22 nov. 2022 à 16:11, Rémy Maucherat <re...@apache.org> a écrit :
> Hi,
>
> Following a post on the user list, I have looked into CDI and
> injection processing in Tomcat standalone (or standalone extended by
> CDI) and found the following issues:
>
> a) metadata-complete is done wrong. The spec got retconned some time
> ago and metadata-complete only means Servlet spec defining metadata,
> such as @WebServlet (= the ones that require scanning all classes just
> in case). So in practice inside DefaultInstanceManager the
> ignoreAnnotations flag shouldn't be used at all and it should simply
> be removed. I am ok on only doing this in 11 ;)
>
> b) CDI is not intertwined with the DefaultInstanceManager. Basically
> as the user said, injections should be done *before* @PostConstruct,
> and it . There's a (minor) problem with the DefaultInstanceManager
> API, a method needs to be protected and then integrations will then be
> able to hack inside the DefaultInstanceManager. You can see this here
> in the OWB integration:
>
> https://github.com/apache/tomcat/blob/main/modules/owb/src/main/java/org/apache/webbeans/web/tomcat/OpenWebBeansInstanceManager.java#L102
> (basically, first call newInstance, because there's no other way, then
> inject, but newInstance will have already invoked @PostConstruct). To
> fix this, I plan to add an Injector interface to
> DefaultInstanceManager since this is pretty much the only way to do
> this cleanly in Tomcat standalone (taking over the whole
> DefaultInstanceManager is clearly not the best idea).
>
Yeah, TomEE ([1]) had some hard time with JSP due to that and its
re-implementation of the instantiation.
Basically the idea was to implement it aside Tomcat default impl but for
some classes it was not convenient enough.
That said I have to admit I'm not sure it needs a new concept (injector)
because basically it will copy the instance manager API (inject, release or
something like that since it is not only about the inject phase so either
you define the lifecycle or you store a Runnable you call at release time
too - isnt the abstraction too complex then?).
Another issue there is that newInstance assumes a "new" which means you can
not use a CDI instance generally speaking, only a banalised instance which
gets injections so it means you can just impl yourself a
newInstance+whatever you want+postconstruct call (this is light actually).
This is where setting ignoreAnnotations would be quite fancy in your own
instance manager and wouldnt need any new API.
That said, enabling to use a CDI instance would be way more powerful and is
not against the spec - it is actually not specified AFAIK.
Indeed it would need a toggle on the OWBIM of Tomcat integration but it
would also open way more doors in terms of usage because your servlet
(filter, ...) is now a CDI beans with a connection to its bus and
interceptors.
I know it can already be done by using a container initializer which gets
beans injected and the instances directly passed to the addServlet()
(instead of the class) but it would also be a very valuable addition to the
module if the instantiation is reworked so I'm just mentionning it as an
opportunity.
[1]
https://github.com/apache/tomee/blob/main/tomee/tomee-catalina/src/main/java/org/apache/tomee/catalina/JavaeeInstanceManager.java
>
> The impact of fixing these for users should be non-existent: it is
> really a Tomcat standalone only thing impacting users with some very
> precise EE needs. A full EE integration will simply take over the
> whole annotation processing and instance manager from Tomcat, and
> hopefully do The Right Thing already.
>
> Although this is not super critical, I plan to address these issues in
> the OWB integration (after adding the needed API change in Tomcat's
> DefaultInstanceManager).
>
> Comments ?
>
> Rémy
>
> ---------------------------------------------------------------------
> To unsubscribe, e-mail: dev-unsubscribe@tomcat.apache.org
> For additional commands, e-mail: dev-help@tomcat.apache.org
>
>