Author: markt
Date: Fri Jun 13 14:27:50 2014
New Revision: 1602443
URL: http://svn.apache.org/r1602443
Log:
When an error occurs after the response has been committed close the connection
immediately rather than attempting to finish the response to make it easier for
the client to differentiate between a complete response and one that failed
part way though.
Modified:
tomcat/tc7.0.x/trunk/ (props changed)
tomcat/tc7.0.x/trunk/java/org/apache/catalina/connector/Response.java
tomcat/tc7.0.x/trunk/java/org/apache/catalina/core/StandardHostValve.java
tomcat/tc7.0.x/trunk/java/org/apache/catalina/core/StandardWrapperValve.java
tomcat/tc7.0.x/trunk/java/org/apache/catalina/valves/ErrorReportValve.java
tomcat/tc7.0.x/trunk/java/org/apache/coyote/ActionCode.java
tomcat/tc7.0.x/trunk/java/org/apache/coyote/ajp/AbstractAjpProcessor.java
tomcat/tc7.0.x/trunk/java/org/apache/coyote/ajp/AjpAprProcessor.java
tomcat/tc7.0.x/trunk/java/org/apache/coyote/ajp/AjpNioProcessor.java
tomcat/tc7.0.x/trunk/java/org/apache/coyote/ajp/AjpProcessor.java
tomcat/tc7.0.x/trunk/java/org/apache/coyote/http11/AbstractHttp11Processor.java
tomcat/tc7.0.x/trunk/test/org/apache/coyote/http11/TestAbstractHttp11Processor.java
tomcat/tc7.0.x/trunk/webapps/docs/changelog.xml
Propchange: tomcat/tc7.0.x/trunk/
------------------------------------------------------------------------------
Merged /tomcat/trunk:r1600449,1600495,1600501,1600579-1600580,1600862,1600965
Modified: tomcat/tc7.0.x/trunk/java/org/apache/catalina/connector/Response.java
URL:
http://svn.apache.org/viewvc/tomcat/tc7.0.x/trunk/java/org/apache/catalina/connector/Response.java?rev=1602443&r1=1602442&r2=1602443&view=diff
==============================================================================
--- tomcat/tc7.0.x/trunk/java/org/apache/catalina/connector/Response.java
(original)
+++ tomcat/tc7.0.x/trunk/java/org/apache/catalina/connector/Response.java Fri
Jun 13 14:27:50 2014
@@ -238,6 +238,7 @@ public class Response
* The error flag.
*/
protected boolean error = false;
+ private boolean errorAfterCommit = false;
/**
@@ -279,6 +280,7 @@ public class Response
appCommitted = false;
included = false;
error = false;
+ errorAfterCommit = false;
isCharacterEncodingSet = false;
if (Globals.IS_SECURITY_ENABLED || Connector.RECYCLE_FACADES) {
@@ -469,7 +471,14 @@ public class Response
* Set the error flag.
*/
public void setError() {
- error = true;
+ if (!error) {
+ error = true;
+ errorAfterCommit = coyoteResponse.isCommitted();
+ Wrapper wrapper = getRequest().getWrapper();
+ if (wrapper != null) {
+ wrapper.incrementErrorCount();
+ }
+ }
}
@@ -481,6 +490,11 @@ public class Response
}
+ public boolean isErrorAfterCommit() {
+ return errorAfterCommit;
+ }
+
+
/**
* Create and return a ServletOutputStream to write the content
* associated with this Response.
@@ -1311,11 +1325,6 @@ public class Response
return;
}
- Wrapper wrapper = getRequest().getWrapper();
- if (wrapper != null) {
- wrapper.incrementErrorCount();
- }
-
setError();
coyoteResponse.setStatus(status);
@@ -1884,7 +1893,4 @@ public class Response
return (sb.toString());
}
-
-
}
-
Modified:
tomcat/tc7.0.x/trunk/java/org/apache/catalina/core/StandardHostValve.java
URL:
http://svn.apache.org/viewvc/tomcat/tc7.0.x/trunk/java/org/apache/catalina/core/StandardHostValve.java?rev=1602443&r1=1602442&r2=1602443&view=diff
==============================================================================
--- tomcat/tc7.0.x/trunk/java/org/apache/catalina/core/StandardHostValve.java
(original)
+++ tomcat/tc7.0.x/trunk/java/org/apache/catalina/core/StandardHostValve.java
Fri Jun 13 14:27:50 2014
@@ -337,7 +337,7 @@ final class StandardHostValve extends Va
request.getRequestURI());
if (custom(request, response, errorPage)) {
try {
- response.flushBuffer();
+ response.finishResponse();
} catch (ClientAbortException e) {
// Ignore
} catch (IOException e) {
@@ -411,7 +411,7 @@ final class StandardHostValve extends Va
realError.getClass());
if (custom(request, response, errorPage)) {
try {
- response.flushBuffer();
+ response.finishResponse();
} catch (IOException e) {
container.getLogger().warn("Exception Processing " +
errorPage, e);
}
Modified:
tomcat/tc7.0.x/trunk/java/org/apache/catalina/core/StandardWrapperValve.java
URL:
http://svn.apache.org/viewvc/tomcat/tc7.0.x/trunk/java/org/apache/catalina/core/StandardWrapperValve.java?rev=1602443&r1=1602442&r2=1602443&view=diff
==============================================================================
---
tomcat/tc7.0.x/trunk/java/org/apache/catalina/core/StandardWrapperValve.java
(original)
+++
tomcat/tc7.0.x/trunk/java/org/apache/catalina/core/StandardWrapperValve.java
Fri Jun 13 14:27:50 2014
@@ -509,7 +509,7 @@ final class StandardWrapperValve
Throwable exception) {
request.setAttribute(RequestDispatcher.ERROR_EXCEPTION, exception);
response.setStatus(HttpServletResponse.SC_INTERNAL_SERVER_ERROR);
-
+ response.setError();
}
public long getProcessingTime() {
Modified:
tomcat/tc7.0.x/trunk/java/org/apache/catalina/valves/ErrorReportValve.java
URL:
http://svn.apache.org/viewvc/tomcat/tc7.0.x/trunk/java/org/apache/catalina/valves/ErrorReportValve.java?rev=1602443&r1=1602442&r2=1602443&view=diff
==============================================================================
--- tomcat/tc7.0.x/trunk/java/org/apache/catalina/valves/ErrorReportValve.java
(original)
+++ tomcat/tc7.0.x/trunk/java/org/apache/catalina/valves/ErrorReportValve.java
Fri Jun 13 14:27:50 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;
@@ -102,6 +103,17 @@ 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
+ try {
+ response.flushBuffer();
+ } catch (Throwable t) {
+ ExceptionUtils.handleThrowable(t);
+ } // Close immediately to signal to the client
that something went
+ // wrong
+ response.getCoyoteResponse().action(ActionCode.CLOSE_NOW,
null);
+ }
return;
}
@@ -112,20 +124,19 @@ public class ErrorReportValve extends Va
return;
}
- if (throwable != null) {
- // The response is an error
- response.setError();
-
- // Reset the response (if possible)
- try {
- response.reset();
- } catch (IllegalStateException e) {
- // Ignore
- }
-
+ if (throwable != null && !response.isError()) {
+ // 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 {
@@ -153,15 +164,14 @@ public class ErrorReportValve extends Va
*/
protected void report(Request request, Response response, Throwable
throwable) {
- // Do nothing on non-HTTP responses
int statusCode = response.getStatus();
// 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()) {
return;
}
-
String message = RequestUtil.filter(response.getMessage());
if (message == null) {
if (throwable != null) {
@@ -287,6 +297,7 @@ public class ErrorReportValve extends Va
// If writer is null, it's an indication that the response has
// been hard committed already, which should never happen
writer.write(sb.toString());
+ response.finishResponse();
}
} catch (IOException e) {
// Ignore
Modified: tomcat/tc7.0.x/trunk/java/org/apache/coyote/ActionCode.java
URL:
http://svn.apache.org/viewvc/tomcat/tc7.0.x/trunk/java/org/apache/coyote/ActionCode.java?rev=1602443&r1=1602442&r2=1602443&view=diff
==============================================================================
--- tomcat/tc7.0.x/trunk/java/org/apache/coyote/ActionCode.java (original)
+++ tomcat/tc7.0.x/trunk/java/org/apache/coyote/ActionCode.java Fri Jun 13
14:27:50 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/tc7.0.x/trunk/java/org/apache/coyote/ajp/AbstractAjpProcessor.java
URL:
http://svn.apache.org/viewvc/tomcat/tc7.0.x/trunk/java/org/apache/coyote/ajp/AbstractAjpProcessor.java?rev=1602443&r1=1602442&r2=1602443&view=diff
==============================================================================
--- tomcat/tc7.0.x/trunk/java/org/apache/coyote/ajp/AbstractAjpProcessor.java
(original)
+++ tomcat/tc7.0.x/trunk/java/org/apache/coyote/ajp/AbstractAjpProcessor.java
Fri Jun 13 14:27:50 2014
@@ -495,6 +495,12 @@ public abstract class AbstractAjpProcess
actionInternal(actionCode, param);
break;
}
+ case CLOSE_NOW: {
+ // Prevent further writes to the response
+ swallowResponse = true;
+ setErrorState(ErrorState.CLOSE_NOW);
+ break;
+ }
}
}
@@ -1111,6 +1117,7 @@ public abstract class AbstractAjpProcess
prepareResponse();
} catch (IOException e) {
setErrorState(ErrorState.CLOSE_NOW);
+ return;
}
}
Modified: tomcat/tc7.0.x/trunk/java/org/apache/coyote/ajp/AjpAprProcessor.java
URL:
http://svn.apache.org/viewvc/tomcat/tc7.0.x/trunk/java/org/apache/coyote/ajp/AjpAprProcessor.java?rev=1602443&r1=1602442&r2=1602443&view=diff
==============================================================================
--- tomcat/tc7.0.x/trunk/java/org/apache/coyote/ajp/AjpAprProcessor.java
(original)
+++ tomcat/tc7.0.x/trunk/java/org/apache/coyote/ajp/AjpAprProcessor.java Fri
Jun 13 14:27:50 2014
@@ -203,7 +203,7 @@ public class AjpAprProcessor extends Abs
}
// Finish the response if not done yet
- if (!finished) {
+ if (!finished && getErrorState().isIoAllowed()) {
try {
finish();
} catch (Throwable t) {
Modified: tomcat/tc7.0.x/trunk/java/org/apache/coyote/ajp/AjpNioProcessor.java
URL:
http://svn.apache.org/viewvc/tomcat/tc7.0.x/trunk/java/org/apache/coyote/ajp/AjpNioProcessor.java?rev=1602443&r1=1602442&r2=1602443&view=diff
==============================================================================
--- tomcat/tc7.0.x/trunk/java/org/apache/coyote/ajp/AjpNioProcessor.java
(original)
+++ tomcat/tc7.0.x/trunk/java/org/apache/coyote/ajp/AjpNioProcessor.java Fri
Jun 13 14:27:50 2014
@@ -190,7 +190,7 @@ public class AjpNioProcessor extends Abs
}
// Finish the response if not done yet
- if (!finished) {
+ if (!finished && getErrorState().isIoAllowed()) {
try {
finish();
} catch (Throwable t) {
Modified: tomcat/tc7.0.x/trunk/java/org/apache/coyote/ajp/AjpProcessor.java
URL:
http://svn.apache.org/viewvc/tomcat/tc7.0.x/trunk/java/org/apache/coyote/ajp/AjpProcessor.java?rev=1602443&r1=1602442&r2=1602443&view=diff
==============================================================================
--- tomcat/tc7.0.x/trunk/java/org/apache/coyote/ajp/AjpProcessor.java (original)
+++ tomcat/tc7.0.x/trunk/java/org/apache/coyote/ajp/AjpProcessor.java Fri Jun
13 14:27:50 2014
@@ -205,7 +205,7 @@ public class AjpProcessor extends Abstra
}
// Finish the response if not done yet
- if (!finished) {
+ if (!finished && getErrorState().isIoAllowed()) {
try {
finish();
} catch (Throwable t) {
Modified:
tomcat/tc7.0.x/trunk/java/org/apache/coyote/http11/AbstractHttp11Processor.java
URL:
http://svn.apache.org/viewvc/tomcat/tc7.0.x/trunk/java/org/apache/coyote/http11/AbstractHttp11Processor.java?rev=1602443&r1=1602442&r2=1602443&view=diff
==============================================================================
---
tomcat/tc7.0.x/trunk/java/org/apache/coyote/http11/AbstractHttp11Processor.java
(original)
+++
tomcat/tc7.0.x/trunk/java/org/apache/coyote/http11/AbstractHttp11Processor.java
Fri Jun 13 14:27:50 2014
@@ -893,6 +893,12 @@ public abstract class AbstractHttp11Proc
getOutputBuffer().finished = true;
break;
}
+ case CLOSE_NOW: {
+ // Block further output
+ getOutputBuffer().finished = true;
+ setErrorState(ErrorState.CLOSE_NOW);
+ break;
+ }
default: {
actionInternal(actionCode, param);
break;
@@ -1138,8 +1144,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) {
@@ -1648,7 +1656,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();
@@ -1746,27 +1754,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/tc7.0.x/trunk/test/org/apache/coyote/http11/TestAbstractHttp11Processor.java
URL:
http://svn.apache.org/viewvc/tomcat/tc7.0.x/trunk/test/org/apache/coyote/http11/TestAbstractHttp11Processor.java?rev=1602443&r1=1602442&r2=1602443&view=diff
==============================================================================
---
tomcat/tc7.0.x/trunk/test/org/apache/coyote/http11/TestAbstractHttp11Processor.java
(original)
+++
tomcat/tc7.0.x/trunk/test/org/apache/coyote/http11/TestAbstractHttp11Processor.java
Fri Jun 13 14:27:50 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();
@@ -84,6 +82,8 @@ public class TestAbstractHttp11Processor
assertTrue(client.isResponse200());
// There should not be an end chunk
assertFalse(client.getResponseBody().endsWith("0"));
+ // The last portion of text should be there
+ assertTrue(client.getResponseBody().endsWith("line03"));
}
private static class ResponseWithErrorServlet extends HttpServlet {
@@ -127,7 +127,7 @@ public class TestAbstractHttp11Processor
Tomcat tomcat = getTomcatInstance();
// Use the normal Tomcat ROOT context
- File root = new File("test/webapp");
+ File root = new File("test/webapp-3.0");
tomcat.addWebapp("", root.getAbsolutePath());
tomcat.start();
Modified: tomcat/tc7.0.x/trunk/webapps/docs/changelog.xml
URL:
http://svn.apache.org/viewvc/tomcat/tc7.0.x/trunk/webapps/docs/changelog.xml?rev=1602443&r1=1602442&r2=1602443&view=diff
==============================================================================
--- tomcat/tc7.0.x/trunk/webapps/docs/changelog.xml (original)
+++ tomcat/tc7.0.x/trunk/webapps/docs/changelog.xml Fri Jun 13 14:27:50 2014
@@ -116,6 +116,12 @@
<bug>56582</bug>: Use switch(actionCode) in processors instead of a
chain of "elseif"s. (kkolinko)
</scode>
+ <fix>
+ When an error occurs after the response has been committed close the
+ connection immediately rather than attempting to finish the response to
+ make it easier for the client to differentiate between a complete
+ response and one that failed part way though. (markt)
+ </fix>
</changelog>
</subsection>
<subsection name="Jasper">
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscr...@tomcat.apache.org
For additional commands, e-mail: dev-h...@tomcat.apache.org
