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
