On Wed, Nov 23, 2022 at 11:10 AM Romain Manni-Bucau <rmannibu...@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 <r...@apache.org> a écrit : > > > On Tue, Nov 22, 2022 at 6:08 PM Romain Manni-Bucau > > <rmannibu...@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 <r...@apache.org> a écrit : > > > > > > > On Tue, Nov 22, 2022 at 4:30 PM Romain Manni-Bucau > > > > <rmannibu...@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 <r...@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-unsubscr...@tomcat.apache.org > > > > > > For additional commands, e-mail: dev-h...@tomcat.apache.org > > > > > > > > > > > > > > > > > > > > --------------------------------------------------------------------- > > > > To unsubscribe, e-mail: dev-unsubscr...@tomcat.apache.org > > > > For additional commands, e-mail: dev-h...@tomcat.apache.org > > > > > > > > > > > > --------------------------------------------------------------------- > > To unsubscribe, e-mail: dev-unsubscr...@tomcat.apache.org > > For additional commands, e-mail: dev-h...@tomcat.apache.org > > > > --------------------------------------------------------------------- To unsubscribe, e-mail: dev-unsubscr...@tomcat.apache.org For additional commands, e-mail: dev-h...@tomcat.apache.org
