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
