Author: markt
Date: Wed Jun 4 19:18:57 2014
New Revision: 1600449
URL: http://svn.apache.org/r1600449
Log:
Improve error handling for an unhandled exception after the response has been
committed. Tomcat will now attempt to:
- flush any unwritten response data to the client
- prevent further writes to the response
- close the connection
This means that the client should experience an unclean close that will enable
them to differentiate (when chunked encoding is used) between an incomplete
response that encountered an error and a complete response that did not (prior
to this commit, Tomcat completed the request normally and - depending on the
response data written to that point - it may not have been visible to the
client that the response was incomplete at that point.
Modified:
tomcat/trunk/java/org/apache/catalina/valves/ErrorReportValve.java
tomcat/trunk/java/org/apache/coyote/ActionCode.java
tomcat/trunk/java/org/apache/coyote/ajp/AbstractAjpProcessor.java
tomcat/trunk/java/org/apache/coyote/http11/AbstractHttp11Processor.java
tomcat/trunk/java/org/apache/coyote/spdy/SpdyProcessor.java
tomcat/trunk/test/org/apache/coyote/http11/TestAbstractHttp11Processor.java
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=1600449&r1=1600448&r2=1600449&view=diff
==============================================================================
--- tomcat/trunk/java/org/apache/catalina/valves/ErrorReportValve.java
(original)
+++ tomcat/trunk/java/org/apache/catalina/valves/ErrorReportValve.java Wed Jun
4 19:18:57 2014
@@ -28,6 +28,7 @@ import org.apache.catalina.connector.Req
import org.apache.catalina.connector.Response;
import org.apache.catalina.util.RequestUtil;
import org.apache.catalina.util.ServerInfo;
+import org.apache.coyote.ActionCode;
import org.apache.tomcat.util.ExceptionUtils;
import org.apache.tomcat.util.res.StringManager;
@@ -77,10 +78,7 @@ public class ErrorReportValve extends Va
// Perform the request
getNext().invoke(request, response);
- if (response.isCommitted()) {
- return;
- }
-
+ // Check the response for an error
Throwable throwable = (Throwable)
request.getAttribute(RequestDispatcher.ERROR_EXCEPTION);
if (request.isAsyncStarted() && ((response.getStatus() < 400 &&
@@ -88,20 +86,33 @@ public class ErrorReportValve extends Va
return;
}
- if (throwable != null) {
- // The response is an error
- response.setError();
+ // If we get this far then there has been an error
- // Reset the response (if possible)
- try {
- response.reset();
- } catch (IllegalStateException e) {
- // Ignore
- }
+ if (response.isCommitted()) {
+ // Flush any data that is still to be written to the client
+ response.flushBuffer();
+ // Mark the response as in error
+ response.setError();
+
response.getCoyoteResponse().setStatus(HttpServletResponse.SC_INTERNAL_SERVER_ERROR);
+ // Close immediately to signal to the client that something went
+ // wrong
+ response.getCoyoteResponse().action(ActionCode.CLOSE_NOW, null);
+ return;
+ }
+ if (throwable != null) {
+ // Make sure that the necessary methods have been called on the
+ // response. (It is possible a component may just have set the
+ // Throwable. Tomcat won't do that but other components might.)
+ // These are safe to call at this point as we know that the
response
+ // has not been committed.
+ response.reset();
response.sendError(HttpServletResponse.SC_INTERNAL_SERVER_ERROR);
}
+ // One way or another, response.sendError() will have been called
before
+ // execution reaches this point and suspended the response. Need to
+ // reverse that so this valve can write to the response.
response.setSuspended(false);
try {
Modified: tomcat/trunk/java/org/apache/coyote/ActionCode.java
URL:
http://svn.apache.org/viewvc/tomcat/trunk/java/org/apache/coyote/ActionCode.java?rev=1600449&r1=1600448&r2=1600449&view=diff
==============================================================================
--- tomcat/trunk/java/org/apache/coyote/ActionCode.java (original)
+++ tomcat/trunk/java/org/apache/coyote/ActionCode.java Wed Jun 4 19:18:57 2014
@@ -32,6 +32,15 @@ public enum ActionCode {
COMMIT,
/**
+ * A serious error occurred from which it is not possible to recover
safely.
+ * Further attempts to write to the response should be ignored and the
+ * connection needs to be closed as soon as possible. This can also be used
+ * to forcibly close a connection if an error occurs after the response has
+ * been committed.
+ */
+ CLOSE_NOW,
+
+ /**
* A flush() operation originated by the client ( i.e. a flush() on the
* servlet output stream or writer, called by a servlet ). Argument is the
* Response.
Modified: tomcat/trunk/java/org/apache/coyote/ajp/AbstractAjpProcessor.java
URL:
http://svn.apache.org/viewvc/tomcat/trunk/java/org/apache/coyote/ajp/AbstractAjpProcessor.java?rev=1600449&r1=1600448&r2=1600449&view=diff
==============================================================================
--- tomcat/trunk/java/org/apache/coyote/ajp/AbstractAjpProcessor.java (original)
+++ tomcat/trunk/java/org/apache/coyote/ajp/AbstractAjpProcessor.java Wed Jun
4 19:18:57 2014
@@ -643,6 +643,12 @@ public abstract class AbstractAjpProcess
getEndpoint().executeNonBlockingDispatches(socketWrapper);
break;
}
+ case CLOSE_NOW: {
+ // Prevent further writes to the response
+ swallowResponse = true;
+ setErrorState(ErrorState.CLOSE_NOW);
+ break;
+ }
}
}
@@ -836,7 +842,7 @@ public abstract class AbstractAjpProcess
}
// Finish the response if not done yet
- if (!finished) {
+ if (!finished && getErrorState().isIoAllowed()) {
try {
finish();
} catch (Throwable t) {
@@ -1541,6 +1547,7 @@ public abstract class AbstractAjpProcess
prepareResponse();
} catch (IOException e) {
setErrorState(ErrorState.CLOSE_NOW);
+ return;
}
}
Modified:
tomcat/trunk/java/org/apache/coyote/http11/AbstractHttp11Processor.java
URL:
http://svn.apache.org/viewvc/tomcat/trunk/java/org/apache/coyote/http11/AbstractHttp11Processor.java?rev=1600449&r1=1600448&r2=1600449&view=diff
==============================================================================
--- tomcat/trunk/java/org/apache/coyote/http11/AbstractHttp11Processor.java
(original)
+++ tomcat/trunk/java/org/apache/coyote/http11/AbstractHttp11Processor.java Wed
Jun 4 19:18:57 2014
@@ -902,6 +902,12 @@ public abstract class AbstractHttp11Proc
getEndpoint().executeNonBlockingDispatches(socketWrapper);
break;
}
+ case CLOSE_NOW: {
+ // Block further output
+ getOutputBuffer().finished = true;
+ setErrorState(ErrorState.CLOSE_NOW);
+ break;
+ }
default: {
actionInternal(actionCode, param);
break;
@@ -1146,8 +1152,10 @@ public abstract class AbstractHttp11Proc
request.updateCounters();
if (!isAsync() && !comet || getErrorState().isError()) {
- getInputBuffer().nextRequest();
- getOutputBuffer().nextRequest();
+ if (getErrorState().isIoAllowed()) {
+ getInputBuffer().nextRequest();
+ getOutputBuffer().nextRequest();
+ }
}
if (!disableUploadTimeout) {
@@ -1668,7 +1676,7 @@ public abstract class AbstractHttp11Proc
RequestInfo rp = request.getRequestProcessor();
try {
rp.setStage(org.apache.coyote.Constants.STAGE_SERVICE);
- if(!getAdapter().asyncDispatch(request, response, status)) {
+ if (!getAdapter().asyncDispatch(request, response, status)) {
setErrorState(ErrorState.CLOSE_NOW);
}
resetTimeouts();
@@ -1742,27 +1750,31 @@ public abstract class AbstractHttp11Proc
public void endRequest() {
// Finish the handling of the request
- try {
- getInputBuffer().endRequest();
- } catch (IOException e) {
- setErrorState(ErrorState.CLOSE_NOW);
- } catch (Throwable t) {
- ExceptionUtils.handleThrowable(t);
- // 500 - Internal Server Error
- // Can't add a 500 to the access log since that has already been
- // written in the Adapter.service method.
- response.setStatus(500);
- setErrorState(ErrorState.CLOSE_NOW);
- getLog().error(sm.getString("http11processor.request.finish"), t);
+ if (getErrorState().isIoAllowed()) {
+ try {
+ getInputBuffer().endRequest();
+ } catch (IOException e) {
+ setErrorState(ErrorState.CLOSE_NOW);
+ } catch (Throwable t) {
+ ExceptionUtils.handleThrowable(t);
+ // 500 - Internal Server Error
+ // Can't add a 500 to the access log since that has already
been
+ // written in the Adapter.service method.
+ response.setStatus(500);
+ setErrorState(ErrorState.CLOSE_NOW);
+ getLog().error(sm.getString("http11processor.request.finish"),
t);
+ }
}
- try {
- getOutputBuffer().endRequest();
- } catch (IOException e) {
- setErrorState(ErrorState.CLOSE_NOW);
- } catch (Throwable t) {
- ExceptionUtils.handleThrowable(t);
- setErrorState(ErrorState.CLOSE_NOW);
- getLog().error(sm.getString("http11processor.response.finish"), t);
+ if (getErrorState().isIoAllowed()) {
+ try {
+ getOutputBuffer().endRequest();
+ } catch (IOException e) {
+ setErrorState(ErrorState.CLOSE_NOW);
+ } catch (Throwable t) {
+ ExceptionUtils.handleThrowable(t);
+ setErrorState(ErrorState.CLOSE_NOW);
+
getLog().error(sm.getString("http11processor.response.finish"), t);
+ }
}
}
Modified: tomcat/trunk/java/org/apache/coyote/spdy/SpdyProcessor.java
URL:
http://svn.apache.org/viewvc/tomcat/trunk/java/org/apache/coyote/spdy/SpdyProcessor.java?rev=1600449&r1=1600448&r2=1600449&view=diff
==============================================================================
--- tomcat/trunk/java/org/apache/coyote/spdy/SpdyProcessor.java (original)
+++ tomcat/trunk/java/org/apache/coyote/spdy/SpdyProcessor.java Wed Jun 4
19:18:57 2014
@@ -409,6 +409,10 @@ public class SpdyProcessor<S> extends Ab
((AtomicBoolean) param).set(asyncStateMachine.isAsyncError());
break;
}
+ case CLOSE_NOW: {
+ setErrorState(ErrorState.CLOSE_NOW);
+ break;
+ }
default: {
// TODO:
// actionInternal(actionCode, param);
Modified:
tomcat/trunk/test/org/apache/coyote/http11/TestAbstractHttp11Processor.java
URL:
http://svn.apache.org/viewvc/tomcat/trunk/test/org/apache/coyote/http11/TestAbstractHttp11Processor.java?rev=1600449&r1=1600448&r2=1600449&view=diff
==============================================================================
--- tomcat/trunk/test/org/apache/coyote/http11/TestAbstractHttp11Processor.java
(original)
+++ tomcat/trunk/test/org/apache/coyote/http11/TestAbstractHttp11Processor.java
Wed Jun 4 19:18:57 2014
@@ -40,7 +40,6 @@ import static org.junit.Assert.assertFal
import static org.junit.Assert.assertTrue;
import org.junit.Assert;
-import org.junit.Ignore;
import org.junit.Test;
import org.apache.catalina.Context;
@@ -54,7 +53,6 @@ import org.apache.tomcat.util.buf.ByteCh
public class TestAbstractHttp11Processor extends TomcatBaseTest {
@Test
- @Ignore
public void testResponseWithErrorChunked() throws Exception {
Tomcat tomcat = getTomcatInstance();
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscr...@tomcat.apache.org
For additional commands, e-mail: dev-h...@tomcat.apache.org
