tomcat-dev mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From ma...@apache.org
Subject svn commit: r1600449 - in /tomcat/trunk: java/org/apache/catalina/valves/ java/org/apache/coyote/ java/org/apache/coyote/ajp/ java/org/apache/coyote/http11/ java/org/apache/coyote/spdy/ test/org/apache/coyote/http11/
Date Wed, 04 Jun 2014 19:18:58 GMT
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-unsubscribe@tomcat.apache.org
For additional commands, e-mail: dev-help@tomcat.apache.org


Mime
View raw message