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? > > 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 > >
