On Thu, Nov 24, 2022 at 6:15 PM Romain Manni-Bucau <rmannibu...@gmail.com> wrote: > > Le jeu. 24 nov. 2022 à 16:58, Rémy Maucherat <r...@apache.org> a écrit : > > > On Thu, Nov 24, 2022 at 10:19 AM Romain Manni-Bucau > > <rmannibu...@gmail.com> wrote: > > > > > > Le jeu. 24 nov. 2022 à 10:13, Rémy Maucherat <r...@apache.org> a écrit : > > > > > > > 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. > > > > > > > > > > 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 <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 > > > > > > > > > > > > --------------------------------------------------------------------- > > 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
