| Summary: | ServletRequestListener.requestInitialized() and ServletRequestListener.requestDestroyed() called in different threads | ||
|---|---|---|---|
| Product: | Tomcat 7 | Reporter: | Jozef Hartinger <jozefhartinger> |
| Component: | Servlet & JSP API | Assignee: | Tomcat Developers Mailing List <dev> |
| Status: | RESOLVED WONTFIX | ||
| Severity: | normal | CC: | mkouba |
| Priority: | P2 | ||
| Version: | 7.0.57 | ||
| Target Milestone: | --- | ||
| Hardware: | PC | ||
| OS: | Linux | ||
*** Bug 57315 has been marked as a duplicate of this bug. *** As with JBoss Web, you're not supposed to be using the request listener but the ThreadBindingListener that I added if you want container integration (although it is not used yet for upgrade mode, I'll do it someday). The request listener is a dead end for your use. The Servlet Specification is not clear on the expected interaction between a ServletRequestListener and async requests. The relevant references I could find were: <quote source="Table 11-3"> Lifecycle | A servlet request has started being processed by Web components. | javax.servlet.ServletRequestListener </quote> <quote source="ServletRequestListener Javadoc"> Interface for receiving notification events about requests coming into and going out of scope of a web application. A ServletRequest is defined as coming into scope of a web application when it is about to enter the first servlet or filter of the web application, and as going out of scope as it exits the last servlet or the first filter in the chain. </quote> That is all pretty clear prior to Servlet 3.0 async. With async, I agree it gets murkier. As far as I am aware, downstream users of Tomcat have passed the TCK without any changes in this area so there is no clarification to be obtained from that source. Tomcat's interpretation of the current language is that a request that is in async mode is still "in the scope of the web application" until the dispatch() or complete() call has been processed. I appreciate the issues this causes for the use of ThreadLocals but the current behaviour is not going to change without some clarification from the Servlet EG. It is usual that, when such clarification is made, the changes are back-ported to earlier versions so if this is clarified in Servlet 4.0, any changes should get back-ported to Tomcat 8 and Tomcat 7. Obviously, one possible result of asking for clarification is that the EG confirms that Tomcat's behaviour is correct. There are use cases for the current behaviour so my personal view is that a new listener is required. Unfortunately, that would make back-porting much less likely (although we could back-port a Tomcat specific listener). I fully agree with the resolution, especially since a facility designed specifically for this use has been added in Tomcat 8. But maybe the ThreadBinding listener should be backported to Tomcat 7 to cover the finer framework integration needs [which include like init, destroy, session expiration, clustering, upgrade, etc etc, which means dealing with lots of different listener types and praying all containers do the "right" thing] ? I am really disappointed by this resolution. First of all: "Interface for receiving notification events about requests coming into and going out of scope of a web application." When a request goes async and the processing is moved somewhere else, it clearly is the case that the processing has gone out scope of a web application (the servlet container knows nothing about where the processing continues). Therefore, the destroyed callback should be called at this point. The request stays out of the scope of the web application until it is dispatched to the web application again. Secondly, ServletRequestListener seems to be the portable hook for integrating various frameworks that use ThreadLocals and it actually is portable across Servlet implementations except for Tomcat. Lastly, nice to know that there is a proprietary hook for doing this. It however still makes integration much more painful compared to the portable one. Would it be possible to consider adding a mode or a configuration option that, when set, would change Tomcat's behavior to align with all the other Servlet implementations? BTW, the documentation for http://tomcat.apache.org/tomcat-8.0-doc/api/org/apache/catalina/ThreadBindingListener.html is miserable. Is there an example somewhere of how this should be registered and when it is called with respect to ServletRequestListener? "is miserable": thanks for the feedback :) "when it is called with respect to ServletRequestListener": "Callback for establishing naming association when entering the application scope. This corresponds to setting the context classloader." So you have it, it is called when calling setContextClassLoader. "Is there an example somewhere of how this should be registered": Same Context.setThreadBindingListener API as in JBoss Web, but you need the usual Tomcat server listener to add it to all contexts as needed. Actually since ThreadBindingListener does not have access to the ServletRequest object (as ServletRequestListener does) it is not really usable for our purpose. Thus, would it be possible to have the configuration option to switch Tomcat to provide aforementioned callback symmetrically with respect to the calling thread? Ah, so you need a request object ? How do you manage during servlet init / destroy then ? Not sure if I fully understand your question. We do not need our stuff to be active during Servlet init/destroy. We need it active during request processing. Well, it's not what I call EE integration then, which would be at the container level, not at the application level limited to Servlets. This is too limited, for example, it has no possibility to work with Websockets (note: in JBoss, it works by accident, and I actually had to back out a useful change because of your hack ...). About the behavior change for ServletRequestListener, it was just resolved as WONTFIX. (In reply to Jozef Hartinger from comment #5) > Secondly, ServletRequestListener seems to be the portable hook for > integrating various frameworks that use ThreadLocals and it actually is > portable across Servlet implementations except for Tomcat. That portability depends entirely on how different vendors interpret what is a very unclear specification. If you want to lobby for consistent behaviour then you need to lobby the Servlet EG. > Would it be possible to consider adding a mode or a configuration option > that, when set, would change Tomcat's behavior to align with all the other > Servlet implementations? I'd prefer to see this discussed by the Servlet EG before taking any action. > Well, it's not what I call EE integration then, which would be at the container > level, not at the application level limited to Servlets. This is too limited, > for example, it has no possibility to work with Websockets Sorry, this is just nonsense. We provide the exact type of integration that the EE specification requires - that is integration with Servlet requests. For different kinds of requests where integration is required, there are other hooks we use. > (note: in JBoss, it > works by accident, and I actually had to back out a useful change because of > your hack ...). Storing data in a ThreadLocal is a hack? It is the only way to implement functionality needed by many frameworks. Unfortunately, unlike other Servlet implementations, Tomcat provides no way of achieving this reliably. > I'd prefer to see this discussed by the Servlet EG before taking any action.
Fair enough, I'll try to bring this up.
Thank you for posting this bug. This will help me to avoid lots of troubles. I am also disappointed by this resolution. I am in the same boat as you as feel that Tomcat should have implemented in a way which matches the implementation by other containers. If I understand it correctly then adding tomcat specific hack will create a dependency on tomcat runtime which will make a framework (which has been coded against Servlet API to keep it portable) un portable. Current implementation makes some people happy while others (who use ThreadLocal (heavily) in a way you have described) unhappy. Implementing it in other way (as done by other containers) will make everyone happy. As Servlet specification does not say which of these two ways are correct, I see no harm in implementing it in other way. If you are using ThreadLocals implementing a Filter is the usual and better approach. 1) A Filter fulfills the same task of being executed when request enters application and when it leaves it. 2) A filter can use try/finally and deal . 3) A filter can be configured with what it is interested with <dispatcher>REQUEST</dispatcher> <dispatcher>FORWARD</dispatcher> <dispatcher>INCLUDE</dispatcher> <dispatcher>ERROR</dispatcher> <dispatcher>ASYNC</dispatcher> Are you trying to emulate a Filter using a "listener"? Does ServletRequestListener serve its own purpose, or it re-implements a filter? Do you expect it to be called on all those dispatches, without being configured in what of them it is interested in? Looking at Tomcat issues in Bugzilla that mention ServletRequestListener Bug 49991 -> login page called by Authenticator Bug 51653 -> dealing with <dispatcher>ERROR</dispatcher> Bug 50789 -> dealing with <dispatcher>ERROR</dispatcher> but it somehow went down to discussion of forwards and to a custom Context option Are we now to discuss <dispatcher>ASYNC</dispatcher>? BTW, The Filter chapter in Servlet Specification explicitly says about "same thread" (<quote>A Filter and the target servlet or resource at the end of the filter chain must execute in the same invocation thread.</quote> and several other phrases). Is there anything about listeners? Does your proposal contradict with use case from bug 51653? Filters are not usable for this purpose as I need to be able to have my code called before other listeners. FYI, we experienced similar symptoms as reported in BUG#57824, the problem may be related. |
TL;DR version: ============== When processing async requests, Tomcat calls ServletRequestListener.requestInitialized() callback from a different thread than the corresponding ServletRequestListener.requestDestroyed() callback. This breaks a lot of frameworks and technologies that depend on ThreadLocals (for example CDI - Weld). All the other Servlet implementations I could get my hands on call these callbacks symmetrically from the same thread. Please fix this. Long version: ============= The Servlet specification is unfortunatelly quite vague when it comes to when and how ServletRequestListeners are called. For a simple synchronous request this is not such a big deal as there are not many options. Both callbacks are called by a thread that processes the request. ServletRequestListener.requestInitialized() at the beginning and ServletRequestListener.requestDestroyed() at the end. So far so good. However, when it comes to asynchronous requests, this gets more complicated. Suppose we have a simple asynchronous servlet that: A1) Does initial processing of the request A2) Goes async (req.startAsync()) A3) Spawns a new thread T that calculates something A4) The servlet exits B1) The spawn thread T calculates the value B2) Once the thread T finishes calculation, it dispatches back to Servlet container for rendering (ctx.dispatch("/asyncFinish?value=calculatedValue")) C1) Servlet container creates response and completes the request A* steps are performed by a thread that initially handles the request. B* steps are performed by a thread spawned by a Servlet, C* steps are performed again by a Servlet container thread. Now the question arises: When and how should ServletRequestListener callbacks be invoked? Common requirements are: 1) Symmetry - each time ServletRequestListener.requestInitialized() is called, there should be a corresponding ServletRequestListener.requestDestroyed() call This I think is clear - if a listener starts something, it should get a chance to stop it. 2) Symmetry with respect to calling thread - each time ServletRequestListener.requestInitialized() is called by thread T, there should be a corresponding ServletRequestListener.requestDestroyed() call by thread T This is an extension of requirement (1) and is needed because frameworks often use ServletRequestListener.requestInitialized() as a hook to set a ThreadLocal for the duration of the request and then unset it in ServletRequestListener.requestDestroyed() Other Servlet containers ======================== As we saw before, the fact that a request is processed in multiple threads may seem in conflict with this second requirement (Symmetry with respect to calling thread - each time). How do other Servlet implementations solve this? They typically do the following: Thread A calls ServletRequestListener.requestInitialized() A1, A2, A3 and A4 are performed Thread A calls ServletRequestListener.requestDestroyed() When the value is calculated by thread B and the request is dispatched to the Servlet container again: Thread C calls ServletRequestListener.requestInitialized() C1 is performed Thread C calls ServletRequestListener.requestDestroyed() This ensures symmetry with respect to a given thread. Tomcat ====== Tomcat simply does: - Thread A calls ServletRequestListener.requestInitialized() - the entire request is processed in threads A, B and C - Thread C calls ServletRequestListener.requestDestroyed() which breaks the second requirement (Symmetry with respect to calling thread). I tested with Tomcat 7 and Tomcat 8 I tested with the following Servlet containers: - Undertow - JBoss Web - Jetty - Grizzly and Tomcat is really the odd one out in not respecting the second requirement. Please fix this so that Tomcat matches the second requirement.