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