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 2014/08/07 10:53:02 UTC
svn commit: r1616441 - in /tomcat/trunk: java/org/apache/catalina/connector/
java/org/apache/catalina/core/ java/org/apache/catalina/valves/
test/org/apache/catalina/core/ webapps/docs/
Author: markt
Date: Thu Aug 7 08:53:01 2014
New Revision: 1616441
URL: http://svn.apache.org/r1616441
Log:
Fix https://issues.apache.org/bugzilla/show_bug.cgi?id=56739
If an application handles an error on an application thread during asynchronous processing by calling HttpServletResponse.sendError(), then ensure that the application is given an opportunity to report that error via an appropriate application defined error page if one is configured.
Modified:
tomcat/trunk/java/org/apache/catalina/connector/CoyoteAdapter.java
tomcat/trunk/java/org/apache/catalina/connector/Response.java
tomcat/trunk/java/org/apache/catalina/core/LocalStrings.properties
tomcat/trunk/java/org/apache/catalina/core/StandardHostValve.java
tomcat/trunk/java/org/apache/catalina/valves/ErrorReportValve.java
tomcat/trunk/test/org/apache/catalina/core/TestAsyncContextImpl.java
tomcat/trunk/webapps/docs/changelog.xml
Modified: tomcat/trunk/java/org/apache/catalina/connector/CoyoteAdapter.java
URL: http://svn.apache.org/viewvc/tomcat/trunk/java/org/apache/catalina/connector/CoyoteAdapter.java?rev=1616441&r1=1616440&r2=1616441&view=diff
==============================================================================
--- tomcat/trunk/java/org/apache/catalina/connector/CoyoteAdapter.java (original)
+++ tomcat/trunk/java/org/apache/catalina/connector/CoyoteAdapter.java Thu Aug 7 08:53:01 2014
@@ -400,6 +400,14 @@ public class CoyoteAdapter implements Ad
}
}
+ // Has an error occurred during async processing that needs to be
+ // processed by the application's error page mechanism (or Tomcat's
+ // if the application doesn't define one)?
+ if (!request.isAsyncDispatching() && request.isAsync() &&
+ response.isErrorReportRequired()) {
+ connector.getService().getContainer().getPipeline().getFirst().invoke(request, response);
+ }
+
if (request.isAsyncDispatching()) {
success = true;
connector.getService().getContainer().getPipeline().getFirst().invoke(request, response);
Modified: tomcat/trunk/java/org/apache/catalina/connector/Response.java
URL: http://svn.apache.org/viewvc/tomcat/trunk/java/org/apache/catalina/connector/Response.java?rev=1616441&r1=1616440&r2=1616441&view=diff
==============================================================================
--- tomcat/trunk/java/org/apache/catalina/connector/Response.java (original)
+++ tomcat/trunk/java/org/apache/catalina/connector/Response.java Thu Aug 7 08:53:01 2014
@@ -32,6 +32,7 @@ import java.util.List;
import java.util.Locale;
import java.util.TimeZone;
import java.util.Vector;
+import java.util.concurrent.atomic.AtomicInteger;
import javax.servlet.ServletOutputStream;
import javax.servlet.SessionTrackingMode;
@@ -195,10 +196,35 @@ public class Response
private boolean isCharacterEncodingSet = false;
/**
- * The error flag.
+ * With the introduction of async processing and the possibility of
+ * non-container threads calling sendError() tracking the current error
+ * state and ensuring that the correct error page is called becomes more
+ * complicated. This state attribute helps by tracking the current error
+ * state and informing callers that attempt to change state if the change
+ * was successful or if another thread got there first.
+ *
+ * <pre>
+ * The state machine is very simple:
+ *
+ * 0 - NONE
+ * 1 - NOT_REPORTED
+ * 2 - REPORTED
+ *
+ *
+ * -->---->-- >NONE
+ * | | |
+ * | | | setError()
+ * ^ ^ |
+ * | | \|/
+ * | |-<-NOT_REPORTED
+ * | |
+ * ^ | report()
+ * | |
+ * | \|/
+ * |----<----REPORTED
+ * </pre>
*/
- protected boolean error = false;
- private boolean errorAfterCommit = false;
+ private AtomicInteger errorState = new AtomicInteger(0);
/**
@@ -239,8 +265,7 @@ public class Response
usingWriter = false;
appCommitted = false;
included = false;
- error = false;
- errorAfterCommit = false;
+ errorState.set(0);
isCharacterEncodingSet = false;
if (Globals.IS_SECURITY_ENABLED || Connector.RECYCLE_FACADES) {
@@ -387,15 +412,15 @@ public class Response
/**
* Set the error flag.
*/
- public void setError() {
- if (!error) {
- error = true;
- errorAfterCommit = coyoteResponse.isCommitted();
+ public boolean setError() {
+ boolean result = errorState.compareAndSet(0, 1);
+ if (result) {
Wrapper wrapper = getRequest().getWrapper();
if (wrapper != null) {
wrapper.incrementErrorCount();
}
}
+ return result;
}
@@ -403,12 +428,17 @@ public class Response
* Error flag accessor.
*/
public boolean isError() {
- return error;
+ return errorState.get() > 0;
+ }
+
+
+ public boolean isErrorReportRequired() {
+ return errorState.get() == 1;
}
- public boolean isErrorAfterCommit() {
- return errorAfterCommit;
+ public boolean setErrorReported() {
+ return errorState.compareAndSet(1, 2);
}
Modified: tomcat/trunk/java/org/apache/catalina/core/LocalStrings.properties
URL: http://svn.apache.org/viewvc/tomcat/trunk/java/org/apache/catalina/core/LocalStrings.properties?rev=1616441&r1=1616440&r2=1616441&view=diff
==============================================================================
--- tomcat/trunk/java/org/apache/catalina/core/LocalStrings.properties (original)
+++ tomcat/trunk/java/org/apache/catalina/core/LocalStrings.properties Thu Aug 7 08:53:01 2014
@@ -158,6 +158,7 @@ standardEngine.jvmRouteFail=Failed to se
standardEngine.noHost=No Host matches server name {0}
standardEngine.notHost=Child of an Engine must be a Host
standardEngine.notParent=Engine cannot have a parent Container
+standardHost.asyncStateError=An asynchronous request was received for processing that was neither an async dispatch nor an error to process
standardHost.clientAbort=Remote Client Aborted Request, IOException: {0}
standardHost.invalidErrorReportValveClass=Couldn''t load specified error report valve class: {0}
standardHost.noContext=No Context configured to process this request
Modified: tomcat/trunk/java/org/apache/catalina/core/StandardHostValve.java
URL: http://svn.apache.org/viewvc/tomcat/trunk/java/org/apache/catalina/core/StandardHostValve.java?rev=1616441&r1=1616440&r2=1616441&view=diff
==============================================================================
--- tomcat/trunk/java/org/apache/catalina/core/StandardHostValve.java (original)
+++ tomcat/trunk/java/org/apache/catalina/core/StandardHostValve.java Thu Aug 7 08:53:01 2014
@@ -119,10 +119,8 @@ final class StandardHostValve extends Va
request.setAsyncSupported(context.getPipeline().isAsyncSupported());
}
- // Don't fire listeners during async processing
- // If a request init listener throws an exception, the request is
- // aborted
boolean asyncAtStart = request.isAsync();
+ boolean asyncDispatching = request.isAsyncDispatching();
// An async error page may dispatch to another resource. This flag helps
// ensure an infinite error handling loop is not entered
boolean errorAtStart = response.isError();
@@ -131,13 +129,22 @@ final class StandardHostValve extends Va
context.bind(Globals.IS_SECURITY_ENABLED, MY_CLASSLOADER);
if (!asyncAtStart && !context.fireRequestInitEvent(request)) {
- // If a listener fails then request processing stops here.
+ // Don't fire listeners during async processing (the listener
+ // fired for the request that called startAsync()).
+ // If a request init listener throws an exception, the request
+ // is aborted.
return;
}
// Ask this Context to process this request
try {
- context.getPipeline().getFirst().invoke(request, response);
+ if (!asyncAtStart || asyncDispatching) {
+ context.getPipeline().getFirst().invoke(request, response);
+ } else {
+ if (!errorAtStart) {
+ throw new IllegalStateException(sm.getString("standardHost.asyncStateError"));
+ }
+ }
} catch (Throwable t) {
ExceptionUtils.handleThrowable(t);
if (errorAtStart) {
@@ -266,7 +273,7 @@ final class StandardHostValve extends Va
// Look for a default error page
errorPage = context.findErrorPage(0);
}
- if (errorPage != null) {
+ if (errorPage != null && response.setErrorReported()) {
response.setAppCommitted(false);
request.setAttribute(RequestDispatcher.ERROR_STATUS_CODE,
Integer.valueOf(statusCode));
@@ -345,31 +352,33 @@ final class StandardHostValve extends Va
}
if (errorPage != null) {
- response.setAppCommitted(false);
- request.setAttribute(Globals.DISPATCHER_REQUEST_PATH_ATTR,
- errorPage.getLocation());
- request.setAttribute(Globals.DISPATCHER_TYPE_ATTR,
- DispatcherType.ERROR);
- request.setAttribute(RequestDispatcher.ERROR_STATUS_CODE,
- new Integer(HttpServletResponse.SC_INTERNAL_SERVER_ERROR));
- request.setAttribute(RequestDispatcher.ERROR_MESSAGE,
- throwable.getMessage());
- request.setAttribute(RequestDispatcher.ERROR_EXCEPTION,
- realError);
- Wrapper wrapper = request.getWrapper();
- if (wrapper != null) {
- request.setAttribute(RequestDispatcher.ERROR_SERVLET_NAME,
- wrapper.getName());
- }
- request.setAttribute(RequestDispatcher.ERROR_REQUEST_URI,
- request.getRequestURI());
- request.setAttribute(RequestDispatcher.ERROR_EXCEPTION_TYPE,
- realError.getClass());
- if (custom(request, response, errorPage)) {
- try {
- response.finishResponse();
- } catch (IOException e) {
- container.getLogger().warn("Exception Processing " + errorPage, e);
+ if (response.setErrorReported()) {
+ response.setAppCommitted(false);
+ request.setAttribute(Globals.DISPATCHER_REQUEST_PATH_ATTR,
+ errorPage.getLocation());
+ request.setAttribute(Globals.DISPATCHER_TYPE_ATTR,
+ DispatcherType.ERROR);
+ request.setAttribute(RequestDispatcher.ERROR_STATUS_CODE,
+ new Integer(HttpServletResponse.SC_INTERNAL_SERVER_ERROR));
+ request.setAttribute(RequestDispatcher.ERROR_MESSAGE,
+ throwable.getMessage());
+ request.setAttribute(RequestDispatcher.ERROR_EXCEPTION,
+ realError);
+ Wrapper wrapper = request.getWrapper();
+ if (wrapper != null) {
+ request.setAttribute(RequestDispatcher.ERROR_SERVLET_NAME,
+ wrapper.getName());
+ }
+ request.setAttribute(RequestDispatcher.ERROR_REQUEST_URI,
+ request.getRequestURI());
+ request.setAttribute(RequestDispatcher.ERROR_EXCEPTION_TYPE,
+ realError.getClass());
+ if (custom(request, response, errorPage)) {
+ try {
+ response.finishResponse();
+ } catch (IOException e) {
+ container.getLogger().warn("Exception Processing " + errorPage, e);
+ }
}
}
} else {
Modified: tomcat/trunk/java/org/apache/catalina/valves/ErrorReportValve.java
URL: http://svn.apache.org/viewvc/tomcat/trunk/java/org/apache/catalina/valves/ErrorReportValve.java?rev=1616441&r1=1616440&r2=1616441&view=diff
==============================================================================
--- tomcat/trunk/java/org/apache/catalina/valves/ErrorReportValve.java (original)
+++ tomcat/trunk/java/org/apache/catalina/valves/ErrorReportValve.java Thu Aug 7 08:53:01 2014
@@ -79,9 +79,10 @@ public class ErrorReportValve extends Va
getNext().invoke(request, response);
if (response.isCommitted()) {
- if (response.isErrorAfterCommit()) {
- // Attempt to flush any data that is still to be written to the
- // client
+ if (response.setErrorReported()) {
+ // Error wasn't previously reported but we can't write an error
+ // page because the response has already been committed. Attempt
+ // to flush any data that is still to be written to the client.
try {
response.flushBuffer();
} catch (Throwable t) {
@@ -146,7 +147,8 @@ public class ErrorReportValve extends Va
// Do nothing on a 1xx, 2xx and 3xx status
// Do nothing if anything has been written already
// Do nothing if the response hasn't been explicitly marked as in error
- if (statusCode < 400 || response.getContentWritten() > 0 || !response.isError()) {
+ // and that error has not been reported.
+ if (statusCode < 400 || response.getContentWritten() > 0 || !response.setErrorReported()) {
return;
}
String message = RequestUtil.filter(response.getMessage());
Modified: tomcat/trunk/test/org/apache/catalina/core/TestAsyncContextImpl.java
URL: http://svn.apache.org/viewvc/tomcat/trunk/test/org/apache/catalina/core/TestAsyncContextImpl.java?rev=1616441&r1=1616440&r2=1616441&view=diff
==============================================================================
--- tomcat/trunk/test/org/apache/catalina/core/TestAsyncContextImpl.java (original)
+++ tomcat/trunk/test/org/apache/catalina/core/TestAsyncContextImpl.java Thu Aug 7 08:53:01 2014
@@ -1302,14 +1302,11 @@ public class TestAsyncContextImpl extend
assertEquals(HttpServletResponse.SC_BAD_REQUEST, rc);
- // SRV 10.9.2 - Writing the response is entirely the application's
+ // SRV 10.9.2 - Handling an error is entirely the application's
// responsibility when an error occurs on an application thread.
- // The test servlet writes no content in this case.
- if (threaded) {
- assertEquals(0, res.getLength());
- } else {
- assertTrue(res.getLength() > 0);
- }
+ // Calling sendError() followed by complete() and expecting the standard
+ // error page mechanism to kick in could be viewed as handling the error
+ assertTrue(res.getLength() > 0);
// Without this test may complete before access log has a chance to log
// the request
Modified: tomcat/trunk/webapps/docs/changelog.xml
URL: http://svn.apache.org/viewvc/tomcat/trunk/webapps/docs/changelog.xml?rev=1616441&r1=1616440&r2=1616441&view=diff
==============================================================================
--- tomcat/trunk/webapps/docs/changelog.xml (original)
+++ tomcat/trunk/webapps/docs/changelog.xml Thu Aug 7 08:53:01 2014
@@ -81,6 +81,13 @@
Cédric Couralet. (markt)
</fix>
<fix>
+ <bug>56739</bug>: If an application handles an error on an application
+ thread during asynchronous processing by calling
+ <code>HttpServletResponse.sendError()</code>, then ensure that the
+ application is given an opportunity to report that error via an
+ appropriate application defined error page if one is configured. (markt)
+ </fix>
+ <fix>
<bug>56784</bug>: Fix a couple of rare but theoretically possible
atomicity bugs. (markt)
</fix>
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@tomcat.apache.org
For additional commands, e-mail: dev-help@tomcat.apache.org