Hi Christian, Actually I was going to respond this morning but got side tracked.
Anyways, I agree that the servlet module needs to provide all of this functionality, however I think we need to make sure that it's more scalable than the way this worked in solder/seam3. The biggest gripe I hear from people about seam3 was that it was all or nothing. It was difficult to activate only portions of the functionality. I think we started to do that in DS with the deactivateable function but there are certain places where it makes sense to deactivate in other ways. What I'm suggesting is that we: 1. Separate the listeners out - can we have two listeners, one for the context listener and one for the session listener. 2. Come up with some way that the filters can be more manageable. For example, maybe I'm only interested in request, not response. Maybe I only want Initialized and not Destroyed. This way we don't have to fire the event every single request. This is probably a good case for deactivatable, but more at the class feature level. 3. Come up with a way to make this not automatically installed, even if you include the servlet module in your archive. if metadata complete is the best option, we should just document the use of metadata complete to disable the installation. John On Fri, Jun 7, 2013 at 11:23 AM, Christian Kaltepoth <christ...@kaltepoth.de > wrote: > Sorry, I think my mail wasn't very clear. Sorry for the confusion. :) > > The servlet module provides two features: the injection of servlet objects > and the propagation of servlet events. It makes sense that the request > events can be disabled. But IMHO the producers that allow injection of > servlet objects is a fundamental feature of the servlet module without any > performance overhead and should therefore not be deactivateable. > > The filter implemented in the servlet modules does two things: > > - It stores the request and the response in a ThreadLocal. The producer > methods use this ThreadLocal to access the request/response. So the > request/response injection requires this filter to work correctly. > - The filter also propagates the servlet events to the CDI event bus. > This is something that should be deactivatable. > > The same applies to things like the ServletContextListener. This one stores > the ServletContext in a map for each context class loader and it sends the > events for it's construction and destruction to the event bus, while only > the latter one should be deactivateable. > > What I wanted to say in my previous mail is that we cannot use a > ServletContainerInitializer > which register the filter only if the servlet events are not disabled, > because this would not only disable the servlet events but also break the > injection of the request/response into CDI beans. > > So I think it would make sense to always register the filter using > web-fragment.xml. Additionally we could control if the events are fired or > not using the config resolver approach that you talked about. But the > ThreadLocal management should always be active so that the injection of > request/response works like expected. > > To sum it up: IMHO we will _always_ need the filters and the listeners in > the servlet module, but we should allow to disable certain parts of their > functionality. > > I hope this makes everything a bit clearer. :) > > Christian > > > > > 2013/6/7 Romain Manni-Bucau <rmannibu...@gmail.com> > > > If a user deactivate it it means it doesn't it the feature so no need of > > any thread local. If a module needs it it can just override the > > configuration to force it (through config resolver) so i still think my > > proposal was possible and better than having it always on (if i missed > sthg > > just push a bit more your explanations please). > > > > *Romain Manni-Bucau* > > *Twitter: @rmannibucau <https://twitter.com/rmannibucau>* > > *Blog: **http://rmannibucau.wordpress.com/*< > > http://rmannibucau.wordpress.com/> > > *LinkedIn: **http://fr.linkedin.com/in/rmannibucau* > > *Github: https://github.com/rmannibucau* > > > > > > > > 2013/6/7 Christian Kaltepoth <christ...@kaltepoth.de> > > > > > @Romain: > > > > > > The filter and the listeners do not only send the events but also > manage > > > the ThreadLocals required for the producers. So currently it is > required > > to > > > have the filter and the listeners configured for the injection to work. > > > That's why I think that it makes sense to always have the filter and > > > listeners in place and just don't send the events if the user disabled > > > them. And of cause one could use ConfigResolver for that. > > > > > > @Mark: > > > > > > So you say that firing events is expensive even if there are no > observers > > > listening? > > > > > > Generally I like the idea of having DeltaSpike automatically detect > that > > > certain features can be disabled because they are not used. So if > nobody > > > listens for servlet events, the filter could simply never send them. > > > > > > > > > > > > > > > 2013/6/7 Gerhard Petracek <gerhard.petra...@gmail.com> > > > > > > > the general jira-issue for it is [1]. > > > > > > > > regards, > > > > gerhard > > > > > > > > [1] https://issues.apache.org/jira/browse/DELTASPIKE-349 > > > > > > > > > > > > > > > > 2013/6/7 Mark Struberg <strub...@yahoo.de> > > > > > > > > > > > > > > > > > > > Gerhard and me thought about this for quite a few other features as > > > well. > > > > > > > > > > > > > > > Event firing is indeed a bit expensive. Thus we might add a > > > > > > > > > > > > > > > Map<Class<? extends Deactivatable> /*the 'feature'*/, Set<Class<?>> > > > > > /*types which get observed*/ > > > > > > > > > > and utilize @Observes ProcessObserverMethod to check whether there > > is a > > > > > need to activate this feature at all. > > > > > > > > > > In short: if there is no ObserverMethod which @Observes ? extends > > > > > ServletResponse or ServletResponse then we don't need to fire any > CDI > > > > > event. Not sure if this is needed though or whether the Containers > > are > > > > > smart enough to optimize this away themselfs (with a negative cache > > > kind > > > > of > > > > > thingy). > > > > > > > > > > > > > > > LieGrue, > > > > > strub > > > > > > > > > > >________________________________ > > > > > > From: Christian Kaltepoth <christ...@kaltepoth.de> > > > > > >To: dev@deltaspike.apache.org > > > > > >Cc: Mark Struberg <strub...@yahoo.de> > > > > > >Sent: Friday, 7 June 2013, 8:22 > > > > > >Subject: Re: Servlet module prototype > > > > > > > > > > > > > > > > > > > > > > > >I think Nicklas and John fear that firing events for each > > > > > request/response could lead to performance issues!?! > > > > > > > > > > > > > > > > > >But I'm not sure if there will be a noticeable performance impact > if > > > > > there are no observers for the events. I don't think that firing an > > > event > > > > > that nobody observes is expensive. > > > > > > > > > > > > > > > > > >And I also think that Solder didn't provide any way to disable > these > > > > > events (correct me if I'm wrong). Or has there been any reports > > > regarding > > > > > Solder's performance? > > > > > > > > > > > > > > > > > > > > > > > >2013/6/7 Romain Manni-Bucau <rmannibu...@gmail.com> > > > > > > > > > > > >Hi > > > > > >> > > > > > >>in fact i don't get why you would like to be able to deactivate > it. > > > > > >>Basically it is a web *module* so if it is here it is either > needed > > > by > > > > a > > > > > >>dep or you explicitely imported it so you want it. So basically a > > > > > >>configurable (through ConfigResolver) ServletContainerInitializer > > is > > > > just > > > > > >>what we need to be able to deactivate not needed listeners. Other > > > > config > > > > > >>can break modules relying on it so it could prevent lib to use > this > > > > > module. > > > > > >> > > > > > >>*Romain Manni-Bucau* > > > > > >>*Twitter: @rmannibucau <https://twitter.com/rmannibucau>* > > > > > >>*Blog: **http://rmannibucau.wordpress.com/*< > > > > > http://rmannibucau.wordpress.com/> > > > > > >>*LinkedIn: **http://fr.linkedin.com/in/rmannibucau* > > > > > >>*Github: https://github.com/rmannibucau* > > > > > >> > > > > > >> > > > > > >> > > > > > >> > > > > > >>2013/6/7 Christian Kaltepoth <christ...@kaltepoth.de> > > > > > >> > > > > > >>> The servlet module doesn't work at all without the filter and > the > > > > > >>> listeners. So I think it absolutely makes sense to include them > > in > > > a > > > > > >>> web-fragment.xml inside the servlet-impl module. If something > > like > > > > > >>> filter/listener ordering is an issue for users, they can still > > set > > > > > >>> "metadata-complete" and manually add the required entries into > > > their > > > > > own > > > > > >>> web.xml. Or they could use <absolute-ordering>. > > > > > >>> > > > > > >>> But I agree that it should be possible to disable the events > (all > > > > > events or > > > > > >>> perhaps just the request/response events?). The question is > which > > > way > > > > > the > > > > > >>> user should use to configure this. Of cause we could also use a > > > > servlet > > > > > >>> context parameter. Something like: > > > > > >>> > > > > > >>> <context-param> > > > > > >>> > > > > > > <param-name>org.apache.deltaspike.DISABLE_SERVLET_EVENTS</param-name> > > > > > >>> <param-value>true</param-value> > > > > > >>> </context-param> > > > > > >>> > > > > > >>> But DeltaSpike already provides a mechanism for disabling > > features > > > > > which is > > > > > >>> part of the core module and is already used in various other > > > places. > > > > > If we > > > > > >>> allow users to deactivate features, we should be consistent in > > how > > > > > users > > > > > >>> can do it. > > > > > >>> > > > > > >>> Is it correct that there is currently no implementation of > > > > > ClassDeactivator > > > > > >>> in DeltaSpike at all? What about adding an implementation that > > uses > > > > > servlet > > > > > >>> context parameters to the servlet module? Wouldn't this be a > nice > > > > > >>> enhancement? This way users could either use "simple" servlet > > > context > > > > > >>> parameters or they could use some other more flexible way if > they > > > > want > > > > > to. > > > > > >>> > > > > > >>> Christian > > > > > >>> > > > > > >>> > > > > > >>> > > > > > >>> > > > > > >>> 2013/6/6 John D. Ament <john.d.am...@gmail.com> > > > > > >>> > > > > > >>> > What if the web-fragment.xml were in a separate JAR? > > > > > >>> > Deactivatable is a custom solution, I'd love to avoid using > it. > > > > > >>> > > > > > > >>> > > > > > > >>> > On Thu, Jun 6, 2013 at 11:03 AM, Christian Kaltepoth < > > > > > >>> > christ...@kaltepoth.de > > > > > >>> > > wrote: > > > > > >>> > > > > > > >>> > > @John, @Nicklas: > > > > > >>> > > > > > > > >>> > > I agree that it should be possible to disable the servlet > > > events. > > > > > But I > > > > > >>> > > think we should automatically register the filter and the > > > > listeners > > > > > >>> using > > > > > >>> > > web-fragment.xml because they are required for the > injection > > to > > > > > work > > > > > >>> > > correctly. > > > > > >>> > > > > > > > >>> > > Isn't this a perfect use case for Deactivatable? We could > > > simply > > > > > >>> define a > > > > > >>> > > dummy implementation of Deactivatable which can be used to > > > > disable > > > > > just > > > > > >>> > the > > > > > >>> > > events. We already do something with GlobalAlternative: > > > > > >>> > > > > > > > >>> > > > > > > > >>> > > > > > > > >>> > > > > > > >>> > > > > > > > > > > > > > > > https://github.com/apache/deltaspike/blob/master/deltaspike/core/impl/src/main/java/org/apache/deltaspike/core/impl/exclude/GlobalAlternative.java#L26 > > > > > >>> > > > > > > > >>> > > > > > > > >>> > > > > > > > >>> > > > > > > >>> > > > > > > > > > > > > > > > https://github.com/apache/deltaspike/blob/master/deltaspike/core/impl/src/main/java/org/apache/deltaspike/core/impl/exclude/extension/ExcludeExtension.java#L91 > > > > > >>> > > > > > > > >>> > > What about this: > > > > > >>> > > > > > > > >>> > > public interface ServletEventBridge implements > > Deactivatable > > > { > > > > } > > > > > >>> > > > > > > > >>> > > Thoughts? > > > > > >>> > > > > > > > >>> > > Christian > > > > > >>> > > > > > > > >>> > > > > > > > >>> > > > > > > > >>> > > 2013/6/6 John D. Ament <john.d.am...@gmail.com> > > > > > >>> > > > > > > > >>> > > > I'd prefer if we simply didn't include the > web-fragment.xml > > > and > > > > > >>> instead > > > > > >>> > > > provided instructions on the wiki on how to enable them. > > > > > >>> > > > > > > > > >>> > > > > > > > > >>> > > > On Thu, Jun 6, 2013 at 4:37 AM, Nicklas Karlsson < > > > > > nicka...@gmail.com > > > > > >>> > > > > > > >>> > > > wrote: > > > > > >>> > > > > > > > > >>> > > > > I would also drop the @Web-annotation, I think. BTW, > can > > > the > > > > > >>> > > > > request/reponse lifecycle events be disabled? I would > > > assume > > > > > that > > > > > >>> > there > > > > > >>> > > > is > > > > > >>> > > > > a lot of firing going off for an ajax-application and > > they > > > > > >>> observers > > > > > >>> > > will > > > > > >>> > > > > have to be resolved even if there are no observers(?) > > > > > >>> > > > > > > > > > >>> > > > > > > > > > >>> > > > > On Thu, Jun 6, 2013 at 11:25 AM, Mark Struberg < > > > > > strub...@yahoo.de> > > > > > >>> > > > wrote: > > > > > >>> > > > > > > > > > >>> > > > > > Nice work! > > > > > >>> > > > > > > > > > > >>> > > > > > The @Web Qualifier looks a bit odd, but will turn out > > > into > > > > > the > > > > > >>> > > broader > > > > > >>> > > > > > discussion about how to implement features which > might > > > get > > > > > >>> 'added' > > > > > >>> > in > > > > > >>> > > > > > future specs. > > > > > >>> > > > > > > > > > > >>> > > > > > > > > > > >>> > > > > > LieGrue, > > > > > >>> > > > > > strub > > > > > >>> > > > > > > > > > > >>> > > > > > > > > > > >>> > > > > > > > > > > >>> > > > > > ----- Original Message ----- > > > > > >>> > > > > > > From: Christian Kaltepoth <christ...@kaltepoth.de> > > > > > >>> > > > > > > To: "deltaspike-...@incubator.apache.org" < > > > > > >>> > > > > > deltaspike-...@incubator.apache.org> > > > > > >>> > > > > > > Cc: > > > > > >>> > > > > > > Sent: Thursday, 6 June 2013, 6:56 > > > > > >>> > > > > > > Subject: Servlet module prototype > > > > > >>> > > > > > > > > > > > >>> > > > > > > Hey all, > > > > > >>> > > > > > > > > > > > >>> > > > > > > I spent some time working on a first version of the > > > > servlet > > > > > >>> > module. > > > > > >>> > > > All > > > > > >>> > > > > > the > > > > > >>> > > > > > > basic features are finished now and therefore I > think > > > its > > > > > time > > > > > >>> to > > > > > >>> > > ask > > > > > >>> > > > > for > > > > > >>> > > > > > > some feedback. > > > > > >>> > > > > > > > > > > > >>> > > > > > > I pushed the code to the "servlet" branch on my > > github > > > > > repo: > > > > > >>> > > > > > > > > > > > >>> > > > > > > https://github.com/chkal/deltaspike/tree/servlet > > > > > >>> > > > > > > > > > > > >>> > > > > > > The servlet module provides two major features: > > > > > >>> > > > > > > > > > > > >>> > > > > > > * Injection of servlet various objects > > > > > >>> > > > > > > * Servlet lifecycle events > > > > > >>> > > > > > > > > > > > >>> > > > > > > The producers are using the qualifier @Web to avoid > > > > > conflicts > > > > > >>> > with > > > > > >>> > > > CDI > > > > > >>> > > > > > 1.1. > > > > > >>> > > > > > > We could discuss whether some other name for the > > > > qualifier > > > > > fits > > > > > >>> > > > better. > > > > > >>> > > > > > > > > > > > >>> > > > > > > See the following classes from the tests to get an > > idea > > > > of > > > > > what > > > > > >>> > can > > > > > >>> > > > be > > > > > >>> > > > > > > injected: > > > > > >>> > > > > > > > > > > > >>> > > > > > > > > > > > >>> > > > > > > > > > > >>> > > > > > > > > > >>> > > > > > > > > >>> > > > > > > > >>> > > > > > > >>> > > > > > > > > > > > > > > > https://github.com/chkal/deltaspike/blob/servlet/deltaspike/modules/servlet/impl/src/test/java/org/apache/deltaspike/test/servlet/impl/producer/ServletObjectInjectionBean.java > > > > > >>> > > > > > > > > > > > >>> > > > > > > > > > > > >>> > > > > > > > > > > >>> > > > > > > > > > >>> > > > > > > > > >>> > > > > > > > >>> > > > > > > >>> > > > > > > > > > > > > > > > https://github.com/chkal/deltaspike/blob/servlet/deltaspike/modules/servlet/impl/src/test/java/org/apache/deltaspike/test/servlet/impl/producer/ServletContextInjectionTest.java > > > > > >>> > > > > > > > > > > > >>> > > > > > > The lifecycle events are fired using the qualifiers > > > > > >>> @Initialized > > > > > >>> > or > > > > > >>> > > > > > > @Destroyed just like in Seam Solder. I also added > the > > > > @Web > > > > > >>> > > qualifier > > > > > >>> > > > to > > > > > >>> > > > > > all > > > > > >>> > > > > > > events, but we could discuss whether this is really > > > > > necessary. > > > > > >>> > > > > > > > > > > > >>> > > > > > > The following classes show which events can be > > > observed: > > > > > >>> > > > > > > > > > > > >>> > > > > > > > > > > > >>> > > > > > > > > > > >>> > > > > > > > > > >>> > > > > > > > > >>> > > > > > > > >>> > > > > > > >>> > > > > > > > > > > > > > > > https://github.com/chkal/deltaspike/blob/servlet/deltaspike/modules/servlet/impl/src/test/java/org/apache/deltaspike/test/servlet/impl/event/request/RequestResponseEventsObserver.java > > > > > >>> > > > > > > > > > > > >>> > > > > > > > > > > > >>> > > > > > > > > > > >>> > > > > > > > > > >>> > > > > > > > > >>> > > > > > > > >>> > > > > > > >>> > > > > > > > > > > > > > > > https://github.com/chkal/deltaspike/blob/servlet/deltaspike/modules/servlet/impl/src/test/java/org/apache/deltaspike/test/servlet/impl/event/session/SessionEventsObserver.java > > > > > >>> > > > > > > > > > > > >>> > > > > > > > > > > > >>> > > > > > > > > > > >>> > > > > > > > > > >>> > > > > > > > > >>> > > > > > > > >>> > > > > > > >>> > > > > > > > > > > > > > > > https://github.com/chkal/deltaspike/blob/servlet/deltaspike/modules/servlet/impl/src/test/java/org/apache/deltaspike/test/servlet/impl/event/context/ServletContextEventsObserver.java > > > > > >>> > > > > > > > > > > > >>> > > > > > > One thing that I'm not quite happy with is the way > > the > > > > > >>> > > ServletContext > > > > > >>> > > > > > > injection works. I'm currently using a map that > > stores > > > > the > > > > > >>> > > > > ServletContext > > > > > >>> > > > > > > for each context class loader. IMHO this is better > > than > > > > > using > > > > > >>> > > > > > > HttpServletRequest.getServletContext() as it also > > works > > > > for > > > > > >>> > threads > > > > > >>> > > > > > outside > > > > > >>> > > > > > > of a request (like schedulers for example). I also > > > wanted > > > > > to > > > > > >>> > avoid > > > > > >>> > > > > using > > > > > >>> > > > > > > the CDI application scope for storing the > > > ServletContext > > > > to > > > > > >>> avoid > > > > > >>> > > > > > problems > > > > > >>> > > > > > > regarding different implementations of the scope in > > > > regard > > > > > to > > > > > >>> EAR > > > > > >>> > > > > > > packaging. I would be very interested in hearing > your > > > > > thoughts > > > > > >>> on > > > > > >>> > > > this. > > > > > >>> > > > > > :) > > > > > >>> > > > > > > > > > > > >>> > > > > > > One other thing. As I currently don't use any > Servlet > > > 3.0 > > > > > >>> > features, > > > > > >>> > > > the > > > > > >>> > > > > > > module depends on the Servlet 2.5 API. Do you think > > it > > > > > makes > > > > > >>> > sense > > > > > >>> > > to > > > > > >>> > > > > > still > > > > > >>> > > > > > > support Servlet 2.5 or should we require at least > > > Servlet > > > > > 3.0? > > > > > >>> > > > > > > > > > > > >>> > > > > > > Looking forward to your feedback. Especially I > would > > > like > > > > > to > > > > > >>> know > > > > > >>> > > if > > > > > >>> > > > > you > > > > > >>> > > > > > > think that the code should be merged into the > > upstream > > > > > >>> > repository. > > > > > >>> > > :) > > > > > > > > > > > > > > > > > > > > > -- > > > Christian Kaltepoth > > > Blog: http://blog.kaltepoth.de/ > > > Twitter: http://twitter.com/chkal > > > GitHub: https://github.com/chkal > > > > > > > > > -- > Christian Kaltepoth > Blog: http://blog.kaltepoth.de/ > Twitter: http://twitter.com/chkal > GitHub: https://github.com/chkal >
