You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@tomcat.apache.org by ma...@apache.org on 2007/01/01 20:31:30 UTC
svn commit: r491619 - in /tomcat/tc6.0.x/trunk:
java/org/apache/catalina/core/ApplicationDispatcher.java
webapps/docs/changelog.xml
Author: markt
Date: Mon Jan 1 11:31:29 2007
New Revision: 491619
URL: http://svn.apache.org/viewvc?view=rev&rev=491619
Log:
Port RD thread safety patch from TC5
Modified:
tomcat/tc6.0.x/trunk/java/org/apache/catalina/core/ApplicationDispatcher.java
tomcat/tc6.0.x/trunk/webapps/docs/changelog.xml
Modified: tomcat/tc6.0.x/trunk/java/org/apache/catalina/core/ApplicationDispatcher.java
URL: http://svn.apache.org/viewvc/tomcat/tc6.0.x/trunk/java/org/apache/catalina/core/ApplicationDispatcher.java?view=diff&rev=491619&r1=491618&r2=491619
==============================================================================
--- tomcat/tc6.0.x/trunk/java/org/apache/catalina/core/ApplicationDispatcher.java (original)
+++ tomcat/tc6.0.x/trunk/java/org/apache/catalina/core/ApplicationDispatcher.java Mon Jan 1 11:31:29 2007
@@ -100,6 +100,48 @@
}
}
+
+ /**
+ * Used to pass state when the request dispatcher is used. Using instance
+ * variables causes threading issues and state is too complex to pass and
+ * return single ServletRequest or ServletResponse objects.
+ */
+ private class State {
+ State(ServletRequest request, ServletResponse response,
+ boolean including) {
+ this.outerRequest = request;
+ this.outerResponse = response;
+ this.including = including;
+ }
+
+ /**
+ * The outermost request that will be passed on to the invoked servlet.
+ */
+ ServletRequest outerRequest = null;
+
+
+ /**
+ * The outermost response that will be passed on to the invoked servlet.
+ */
+ ServletResponse outerResponse = null;
+
+ /**
+ * The request wrapper we have created and installed (if any).
+ */
+ ServletRequest wrapRequest = null;
+
+
+ /**
+ * The response wrapper we have created and installed (if any).
+ */
+ ServletResponse wrapResponse = null;
+
+ /**
+ * Are we performing an include() instead of a forward()?
+ */
+ boolean including = false;
+ }
+
// ----------------------------------------------------------- Constructors
@@ -131,7 +173,6 @@
this.context = (Context) wrapper.getParent();
this.requestURI = requestURI;
this.servletPath = servletPath;
- this.origServletPath = servletPath;
this.pathInfo = pathInfo;
this.queryString = queryString;
this.name = name;
@@ -153,30 +194,12 @@
private static Log log = LogFactory.getLog(ApplicationDispatcher.class);
/**
- * The request specified by the dispatching application.
- */
- private ServletRequest appRequest = null;
-
-
- /**
- * The response specified by the dispatching application.
- */
- private ServletResponse appResponse = null;
-
-
- /**
* The Context this RequestDispatcher is associated with.
*/
private Context context = null;
/**
- * Are we performing an include() instead of a forward()?
- */
- private boolean including = false;
-
-
- /**
* Descriptive information about this implementation.
*/
private static final String info =
@@ -190,18 +213,6 @@
/**
- * The outermost request that will be passed on to the invoked servlet.
- */
- private ServletRequest outerRequest = null;
-
-
- /**
- * The outermost response that will be passed on to the invoked servlet.
- */
- private ServletResponse outerResponse = null;
-
-
- /**
* The extra path information for this RequestDispatcher.
*/
private String pathInfo = null;
@@ -218,13 +229,13 @@
*/
private String requestURI = null;
+
/**
* The servlet path for this RequestDispatcher.
*/
private String servletPath = null;
- private String origServletPath = null;
-
+
/**
* The StringManager for this package.
*/
@@ -246,18 +257,6 @@
private Wrapper wrapper = null;
- /**
- * The request wrapper we have created and installed (if any).
- */
- private ServletRequest wrapRequest = null;
-
-
- /**
- * The response wrapper we have created and installed (if any).
- */
- private ServletResponse wrapResponse = null;
-
-
// ------------------------------------------------------------- Properties
@@ -323,11 +322,11 @@
}
// Set up to handle the specified request and response
- setup(request, response, false);
+ State state = new State(request, response, false);
if (Globals.STRICT_SERVLET_COMPLIANCE) {
// Check SRV.8.2 / SRV.14.2.5.1 compliance
- checkSameObjects();
+ checkSameObjects(request, response);
}
// Identify the HTTP-specific request and response objects (if any)
@@ -344,7 +343,7 @@
if ( log.isDebugEnabled() )
log.debug(" Non-HTTP Forward");
- processRequest(hrequest,hresponse);
+ processRequest(hrequest,hresponse,state);
}
@@ -355,17 +354,17 @@
log.debug(" Named Dispatcher Forward");
ApplicationHttpRequest wrequest =
- (ApplicationHttpRequest) wrapRequest();
+ (ApplicationHttpRequest) wrapRequest(state);
wrequest.setRequestURI(hrequest.getRequestURI());
wrequest.setContextPath(hrequest.getContextPath());
wrequest.setServletPath(hrequest.getServletPath());
wrequest.setPathInfo(hrequest.getPathInfo());
wrequest.setQueryString(hrequest.getQueryString());
- processRequest(request,response);
+ processRequest(request,response,state);
wrequest.recycle();
- unwrapRequest();
+ unwrapRequest(state);
}
@@ -376,7 +375,7 @@
log.debug(" Path Based Forward");
ApplicationHttpRequest wrequest =
- (ApplicationHttpRequest) wrapRequest();
+ (ApplicationHttpRequest) wrapRequest(state);
String contextPath = context.getPath();
if (hrequest.getAttribute(Globals.FORWARD_REQUEST_URI_ATTR) == null) {
@@ -401,10 +400,10 @@
wrequest.setQueryParams(queryString);
}
- processRequest(request,response);
+ processRequest(request,response,state);
wrequest.recycle();
- unwrapRequest();
+ unwrapRequest(state);
}
@@ -443,32 +442,33 @@
}
-
/**
* Prepare the request based on the filter configuration.
* @param request The servlet request we are processing
* @param response The servlet response we are creating
+ * @param state The RD state
*
* @exception IOException if an input/output error occurs
* @exception ServletException if a servlet error occurs
*/
private void processRequest(ServletRequest request,
- ServletResponse response)
+ ServletResponse response,
+ State state)
throws IOException, ServletException {
Integer disInt = (Integer) request.getAttribute
(ApplicationFilterFactory.DISPATCHER_TYPE_ATTR);
if (disInt != null) {
if (disInt.intValue() != ApplicationFilterFactory.ERROR) {
- outerRequest.setAttribute
+ state.outerRequest.setAttribute
(ApplicationFilterFactory.DISPATCHER_REQUEST_PATH_ATTR,
- origServletPath);
- outerRequest.setAttribute
+ servletPath);
+ state.outerRequest.setAttribute
(ApplicationFilterFactory.DISPATCHER_TYPE_ATTR,
Integer.valueOf(ApplicationFilterFactory.FORWARD));
- invoke(outerRequest, response);
+ invoke(state.outerRequest, response, state);
} else {
- invoke(outerRequest, response);
+ invoke(state.outerRequest, response, state);
}
}
@@ -510,16 +510,15 @@
throws ServletException, IOException
{
// Set up to handle the specified request and response
- setup(request, response, true);
+ State state = new State(request, response, true);
if (Globals.STRICT_SERVLET_COMPLIANCE) {
// Check SRV.8.2 / SRV.14.2.5.1 compliance
- checkSameObjects();
+ checkSameObjects(request, response);
}
// Create a wrapped response to use for this request
- // ServletResponse wresponse = null;
- ServletResponse wresponse = wrapResponse();
+ wrapResponse(state);
// Handle a non-HTTP include
if (!(request instanceof HttpServletRequest) ||
@@ -529,8 +528,10 @@
log.debug(" Non-HTTP Include");
request.setAttribute(ApplicationFilterFactory.DISPATCHER_TYPE_ATTR,
Integer.valueOf(ApplicationFilterFactory.INCLUDE));
- request.setAttribute(ApplicationFilterFactory.DISPATCHER_REQUEST_PATH_ATTR, origServletPath);
- invoke(request, outerResponse);
+ request.setAttribute(
+ ApplicationFilterFactory.DISPATCHER_REQUEST_PATH_ATTR,
+ servletPath);
+ invoke(request, state.outerResponse, state);
}
// Handle an HTTP named dispatcher include
@@ -540,14 +541,16 @@
log.debug(" Named Dispatcher Include");
ApplicationHttpRequest wrequest =
- (ApplicationHttpRequest) wrapRequest();
+ (ApplicationHttpRequest) wrapRequest(state);
wrequest.setAttribute(Globals.NAMED_DISPATCHER_ATTR, name);
if (servletPath != null)
wrequest.setServletPath(servletPath);
wrequest.setAttribute(ApplicationFilterFactory.DISPATCHER_TYPE_ATTR,
Integer.valueOf(ApplicationFilterFactory.INCLUDE));
- wrequest.setAttribute(ApplicationFilterFactory.DISPATCHER_REQUEST_PATH_ATTR, origServletPath);
- invoke(outerRequest, outerResponse);
+ wrequest.setAttribute(
+ ApplicationFilterFactory.DISPATCHER_REQUEST_PATH_ATTR,
+ servletPath);
+ invoke(state.outerRequest, state.outerResponse, state);
wrequest.recycle();
}
@@ -559,7 +562,7 @@
log.debug(" Path Based Include");
ApplicationHttpRequest wrequest =
- (ApplicationHttpRequest) wrapRequest();
+ (ApplicationHttpRequest) wrapRequest(state);
String contextPath = context.getPath();
if (requestURI != null)
wrequest.setAttribute(Globals.INCLUDE_REQUEST_URI_ATTR,
@@ -581,8 +584,10 @@
wrequest.setAttribute(ApplicationFilterFactory.DISPATCHER_TYPE_ATTR,
Integer.valueOf(ApplicationFilterFactory.INCLUDE));
- wrequest.setAttribute(ApplicationFilterFactory.DISPATCHER_REQUEST_PATH_ATTR, origServletPath);
- invoke(outerRequest, outerResponse);
+ wrequest.setAttribute(
+ ApplicationFilterFactory.DISPATCHER_REQUEST_PATH_ATTR,
+ servletPath);
+ invoke(state.outerRequest, state.outerResponse, state);
wrequest.recycle();
}
@@ -608,8 +613,8 @@
* @exception IOException if an input/output error occurs
* @exception ServletException if a servlet error occurs
*/
- private void invoke(ServletRequest request, ServletResponse response)
- throws IOException, ServletException {
+ private void invoke(ServletRequest request, ServletResponse response,
+ State state) throws IOException, ServletException {
// Checking to see if the context classloader is the current context
// classloader. If it's not, we're saving it, and setting the context
@@ -624,7 +629,6 @@
}
// Initialize local variables we may need
- HttpServletRequest hrequest = (HttpServletRequest) request;
HttpServletResponse hresponse = (HttpServletResponse) response;
Servlet servlet = null;
IOException ioException = null;
@@ -758,8 +762,8 @@
// Unwrap request/response if needed
// See Bugzilla 30949
- unwrapRequest();
- unwrapResponse();
+ unwrapRequest(state);
+ unwrapResponse(state);
// Rethrow an exception if one was thrown by the invoked servlet
if (ioException != null)
@@ -773,35 +777,15 @@
/**
- * Set up to handle the specified request and response
- *
- * @param request The servlet request specified by the caller
- * @param response The servlet response specified by the caller
- * @param including Are we performing an include() as opposed to
- * a forward()?
- */
- private void setup(ServletRequest request, ServletResponse response,
- boolean including) {
-
- this.appRequest = request;
- this.appResponse = response;
- this.outerRequest = request;
- this.outerResponse = response;
- this.including = including;
-
- }
-
-
- /**
* Unwrap the request if we have wrapped it.
*/
- private void unwrapRequest() {
+ private void unwrapRequest(State state) {
- if (wrapRequest == null)
+ if (state.wrapRequest == null)
return;
ServletRequest previous = null;
- ServletRequest current = outerRequest;
+ ServletRequest current = state.outerRequest;
while (current != null) {
// If we run into the container request we are done
@@ -810,11 +794,11 @@
break;
// Remove the current request if it is our wrapper
- if (current == wrapRequest) {
+ if (current == state.wrapRequest) {
ServletRequest next =
((ServletRequestWrapper) current).getRequest();
if (previous == null)
- outerRequest = next;
+ state.outerRequest = next;
else
((ServletRequestWrapper) previous).setRequest(next);
break;
@@ -832,13 +816,13 @@
/**
* Unwrap the response if we have wrapped it.
*/
- private void unwrapResponse() {
+ private void unwrapResponse(State state) {
- if (wrapResponse == null)
+ if (state.wrapResponse == null)
return;
ServletResponse previous = null;
- ServletResponse current = outerResponse;
+ ServletResponse current = state.outerResponse;
while (current != null) {
// If we run into the container response we are done
@@ -847,11 +831,11 @@
break;
// Remove the current response if it is our wrapper
- if (current == wrapResponse) {
+ if (current == state.wrapResponse) {
ServletResponse next =
((ServletResponseWrapper) current).getResponse();
if (previous == null)
- outerResponse = next;
+ state.outerResponse = next;
else
((ServletResponseWrapper) previous).setResponse(next);
break;
@@ -870,11 +854,11 @@
* Create and return a request wrapper that has been inserted in the
* appropriate spot in the request chain.
*/
- private ServletRequest wrapRequest() {
+ private ServletRequest wrapRequest(State state) {
// Locate the request we should insert in front of
ServletRequest previous = null;
- ServletRequest current = outerRequest;
+ ServletRequest current = state.outerRequest;
while (current != null) {
if ("org.apache.catalina.servlets.InvokerHttpRequest".
equals(current.getClass().getName()))
@@ -899,11 +883,11 @@
// Compute a crossContext flag
HttpServletRequest hcurrent = (HttpServletRequest) current;
boolean crossContext = false;
- if ((outerRequest instanceof ApplicationHttpRequest) ||
- (outerRequest instanceof Request) ||
- (outerRequest instanceof HttpServletRequest)) {
+ if ((state.outerRequest instanceof ApplicationHttpRequest) ||
+ (state.outerRequest instanceof Request) ||
+ (state.outerRequest instanceof HttpServletRequest)) {
HttpServletRequest houterRequest =
- (HttpServletRequest) outerRequest;
+ (HttpServletRequest) state.outerRequest;
Object contextPath = houterRequest.getAttribute
(Globals.INCLUDE_CONTEXT_PATH_ATTR);
if (contextPath == null) {
@@ -918,10 +902,10 @@
wrapper = new ApplicationRequest(current);
}
if (previous == null)
- outerRequest = wrapper;
+ state.outerRequest = wrapper;
else
((ServletRequestWrapper) previous).setRequest(wrapper);
- wrapRequest = wrapper;
+ state.wrapRequest = wrapper;
return (wrapper);
}
@@ -931,11 +915,11 @@
* Create and return a response wrapper that has been inserted in the
* appropriate spot in the response chain.
*/
- private ServletResponse wrapResponse() {
+ private ServletResponse wrapResponse(State state) {
// Locate the response we should insert in front of
ServletResponse previous = null;
- ServletResponse current = outerResponse;
+ ServletResponse current = state.outerResponse;
while (current != null) {
if (!(current instanceof ServletResponseWrapper))
break;
@@ -956,21 +940,24 @@
(current instanceof HttpServletResponse))
wrapper =
new ApplicationHttpResponse((HttpServletResponse) current,
- including);
+ state.including);
else
- wrapper = new ApplicationResponse(current, including);
+ wrapper = new ApplicationResponse(current, state.including);
if (previous == null)
- outerResponse = wrapper;
+ state.outerResponse = wrapper;
else
((ServletResponseWrapper) previous).setResponse(wrapper);
- wrapResponse = wrapper;
+ state.wrapResponse = wrapper;
return (wrapper);
}
- private void checkSameObjects() throws ServletException {
- ServletRequest originalRequest = ApplicationFilterChain.getLastServicedRequest();
- ServletResponse originalResponse = ApplicationFilterChain.getLastServicedResponse();
+ private void checkSameObjects(ServletRequest appRequest,
+ ServletResponse appResponse) throws ServletException {
+ ServletRequest originalRequest =
+ ApplicationFilterChain.getLastServicedRequest();
+ ServletResponse originalResponse =
+ ApplicationFilterChain.getLastServicedResponse();
// Some forwards, eg from valves will not set original values
if (originalRequest == null || originalResponse == null) {
@@ -980,9 +967,11 @@
boolean same = false;
ServletRequest dispatchedRequest = appRequest;
- //find the bottom most request, the one that was passed into the service method
- while ( originalRequest instanceof ServletRequestWrapper && ((ServletRequestWrapper) originalRequest).getRequest()!=null ) {
- originalRequest = ((ServletRequestWrapper) originalRequest).getRequest();
+ //find the request that was passed into the service method
+ while (originalRequest instanceof ServletRequestWrapper &&
+ ((ServletRequestWrapper) originalRequest).getRequest()!=null ) {
+ originalRequest =
+ ((ServletRequestWrapper) originalRequest).getRequest();
}
//compare with the dispatched request
while (!same) {
@@ -990,21 +979,26 @@
same = true;
}
if (!same && dispatchedRequest instanceof ServletRequestWrapper) {
- dispatchedRequest = ((ServletRequestWrapper) dispatchedRequest).getRequest();
+ dispatchedRequest =
+ ((ServletRequestWrapper) dispatchedRequest).getRequest();
} else {
break;
}
}
if (!same) {
- throw new ServletException(sm.getString("applicationDispatcher.specViolation.request"));
+ throw new ServletException(sm.getString(
+ "applicationDispatcher.specViolation.request"));
}
same = false;
ServletResponse dispatchedResponse = appResponse;
- //find the bottom most response, the one that was passed into the service method
- while ( originalResponse instanceof ServletResponseWrapper && ((ServletResponseWrapper) originalResponse).getResponse()!=null ) {
- originalResponse = ((ServletResponseWrapper) originalResponse).getResponse();
+ //find the response that was passed into the service method
+ while (originalResponse instanceof ServletResponseWrapper &&
+ ((ServletResponseWrapper) originalResponse).getResponse() !=
+ null ) {
+ originalResponse =
+ ((ServletResponseWrapper) originalResponse).getResponse();
}
//compare with the dispatched response
while (!same) {
@@ -1013,14 +1007,16 @@
}
if (!same && dispatchedResponse instanceof ServletResponseWrapper) {
- dispatchedResponse = ((ServletResponseWrapper) dispatchedResponse).getResponse();
+ dispatchedResponse =
+ ((ServletResponseWrapper) dispatchedResponse).getResponse();
} else {
break;
}
}
if (!same) {
- throw new ServletException(sm.getString("applicationDispatcher.specViolation.response"));
+ throw new ServletException(sm.getString(
+ "applicationDispatcher.specViolation.response"));
}
}
}
Modified: tomcat/tc6.0.x/trunk/webapps/docs/changelog.xml
URL: http://svn.apache.org/viewvc/tomcat/tc6.0.x/trunk/webapps/docs/changelog.xml?view=diff&rev=491619&r1=491618&r2=491619
==============================================================================
--- tomcat/tc6.0.x/trunk/webapps/docs/changelog.xml (original)
+++ tomcat/tc6.0.x/trunk/webapps/docs/changelog.xml Mon Jan 1 11:31:29 2007
@@ -15,6 +15,13 @@
<body>
<section name="Tomcat 6.0.8 (remm)">
+ <subsection name="Catalina">
+ <changelog>
+ <fix>
+ Make provided instances of RequestDispatcher thread safe. (markt)
+ </fix>
+ </changelog>
+ </subsection>
<subsection name="Jasper">
<changelog>
<fix>
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@tomcat.apache.org
For additional commands, e-mail: dev-help@tomcat.apache.org
Re: svn commit: r491619 - in /tomcat/tc6.0.x/trunk: java/org/apache/catalina/core/ApplicationDispatcher.java
webapps/docs/changelog.xml
Posted by Remy Maucherat <re...@apache.org>.
Mark Thomas wrote:
> Remy Maucherat wrote:
>> markt@apache.org wrote:
>>> Author: markt
>>> Date: Mon Jan 1 11:31:29 2007
>>> New Revision: 491619
>>>
>>> URL: http://svn.apache.org/viewvc?view=rev&rev=491619
>>> Log:
>>> Port RD thread safety patch from TC5
>> I don't see any justification for this at the moment. Any further info ?
>
> This started with http://marc.theaimsgroup.com/?t=116558790000003&r=1&w=2
>
> The short version of the above thread is that a RD was obtained using
> ServletContext.getNamedDispatcher() and then re-used. When multiple
> requests used the RD at the same time, the request objects got mixed
> up (since the RD is not thread safe) which in turn lead to an
> occasional ISE when both threads tried to call getParameterMap() at
> the same time on the same Request object.
>
> There was some debate whether or not the spec intended RDs to be
> re-used in this way. I couldn't see anything that explicitly
> dis-allowed it; hence I implemented the patch.
I remember the issue (not from this thread).
I don't understand how a RD could be be used by multiple threads, as no
objects provided by the servlet container are thread safe. If you add
some sort of thread safety somewhere, then users will demand that all
the other objects to be thread safe, which is not going to happen. If
they want to reuse the request dispatcher somehow and use it without
checking, then they should sync on it. I thought the pattern was to
obtain a RD every time (which is slower, but that's what I saw pretty
much everywhere).
> There are a couple of places where the patch could be cleaner but I
> opted to make the changes in a consistent way throughout the code so
> it was clearer what the patch was doing. Cleaning this up is on my
> list of things to do but I wanted to give people a chance to consider
> the patch as-is first.
Rémy
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@tomcat.apache.org
For additional commands, e-mail: dev-help@tomcat.apache.org
Re: svn commit: r491619 - in /tomcat/tc6.0.x/trunk: java/org/apache/catalina/core/ApplicationDispatcher.java
webapps/docs/changelog.xml
Posted by Mark Thomas <ma...@apache.org>.
Remy Maucherat wrote:
> markt@apache.org wrote:
>> Author: markt
>> Date: Mon Jan 1 11:31:29 2007
>> New Revision: 491619
>>
>> URL: http://svn.apache.org/viewvc?view=rev&rev=491619
>> Log:
>> Port RD thread safety patch from TC5
>
> I don't see any justification for this at the moment. Any further info ?
This started with http://marc.theaimsgroup.com/?t=116558790000003&r=1&w=2
The short version of the above thread is that a RD was obtained using
ServletContext.getNamedDispatcher() and then re-used. When multiple
requests used the RD at the same time, the request objects got mixed
up (since the RD is not thread safe) which in turn lead to an
occasional ISE when both threads tried to call getParameterMap() at
the same time on the same Request object.
There was some debate whether or not the spec intended RDs to be
re-used in this way. I couldn't see anything that explicitly
dis-allowed it; hence I implemented the patch.
There are a couple of places where the patch could be cleaner but I
opted to make the changes in a consistent way throughout the code so
it was clearer what the patch was doing. Cleaning this up is on my
list of things to do but I wanted to give people a chance to consider
the patch as-is first.
Mark
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@tomcat.apache.org
For additional commands, e-mail: dev-help@tomcat.apache.org
Re: svn commit: r491619 - in /tomcat/tc6.0.x/trunk: java/org/apache/catalina/core/ApplicationDispatcher.java
webapps/docs/changelog.xml
Posted by Remy Maucherat <re...@apache.org>.
markt@apache.org wrote:
> Author: markt
> Date: Mon Jan 1 11:31:29 2007
> New Revision: 491619
>
> URL: http://svn.apache.org/viewvc?view=rev&rev=491619
> Log:
> Port RD thread safety patch from TC5
I don't see any justification for this at the moment. Any further info ?
Rémy
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@tomcat.apache.org
For additional commands, e-mail: dev-help@tomcat.apache.org