tomcat-dev mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From ma...@apache.org
Subject svn commit: r1408148 - in /tomcat/trunk: java/org/apache/catalina/core/AsyncContextImpl.java java/org/apache/catalina/core/StandardHostValve.java java/org/apache/coyote/AsyncStateMachine.java test/org/apache/catalina/core/TestAsyncContextImpl.java
Date Sun, 11 Nov 2012 23:26:58 GMT
Author: markt
Date: Sun Nov 11 23:26:55 2012
New Revision: 1408148

URL: http://svn.apache.org/viewvc?rev=1408148&view=rev
Log:
Fix https://issues.apache.org/bugzilla/show_bug.cgi?id=54123
There are two things going on here:
1. The reported bug. If there is a async timeout and no async listeners, trigger a 500 response.
2. Implement "error dispatch". This is used a couple of times in the spec without any definition.
The implication from the part of the spec quoted in the bug report is:
   - The standard error page mechanism should be used to identify the page
   - An async request that has been started should be left in async mode when forwarding to
the error page
   - The error page may call complete() or dispatch()
This commit hooks into the StandardHostValve to access the error page mechanism. I could have
copied and pasted but I preferred the dependency on StandardHostValve
Because the error page may do a dispatch(), need to ensure that when running the dispatch(),
the error page mechanism is not triggered a second time.
Depending on what emerges running the full unit tests and the TCK, I mat still decide to copy
the error page code to AsyncContextImpl

Modified:
    tomcat/trunk/java/org/apache/catalina/core/AsyncContextImpl.java
    tomcat/trunk/java/org/apache/catalina/core/StandardHostValve.java
    tomcat/trunk/java/org/apache/coyote/AsyncStateMachine.java
    tomcat/trunk/test/org/apache/catalina/core/TestAsyncContextImpl.java

Modified: tomcat/trunk/java/org/apache/catalina/core/AsyncContextImpl.java
URL: http://svn.apache.org/viewvc/tomcat/trunk/java/org/apache/catalina/core/AsyncContextImpl.java?rev=1408148&r1=1408147&r2=1408148&view=diff
==============================================================================
--- tomcat/trunk/java/org/apache/catalina/core/AsyncContextImpl.java (original)
+++ tomcat/trunk/java/org/apache/catalina/core/AsyncContextImpl.java Sun Nov 11 23:26:55 2012
@@ -41,6 +41,8 @@ import javax.servlet.http.HttpServletRes
 import org.apache.catalina.AsyncDispatcher;
 import org.apache.catalina.Context;
 import org.apache.catalina.Globals;
+import org.apache.catalina.Host;
+import org.apache.catalina.Valve;
 import org.apache.catalina.connector.Request;
 import org.apache.coyote.ActionCode;
 import org.apache.coyote.AsyncContextCallback;
@@ -182,8 +184,8 @@ public class AsyncContextImpl implements
                             ActionCode.ASYNC_IS_TIMINGOUT, result);
                     return !result.get();
                 } else {
-                    // No listeners, container calls complete
-                    complete();
+                    // No listeners, trigger error handling
+                    return false;
                 }
 
             } finally {
@@ -423,6 +425,23 @@ public class AsyncContextImpl implements
                         listener.getClass().getName() + "]", ioe);
             }
         }
+
+        // SRV.2.3.3.3 (search for "error dispatch")
+        if (servletResponse instanceof HttpServletResponse) {
+            ((HttpServletResponse) servletResponse).setStatus(
+                    HttpServletResponse.SC_INTERNAL_SERVER_ERROR);
+        }
+
+        Host host = (Host) context.getParent();
+        Valve stdHostValve = host.getPipeline().getBasic();
+        if (stdHostValve instanceof StandardHostValve) {
+            ((StandardHostValve) stdHostValve).throwable(request,
+                    request.getResponse(), t);
+        }
+
+        if (isStarted() && !request.isAsyncDispatching()) {
+            complete();
+        }
     }
 
 

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=1408148&r1=1408147&r2=1408148&view=diff
==============================================================================
--- tomcat/trunk/java/org/apache/catalina/core/StandardHostValve.java (original)
+++ tomcat/trunk/java/org/apache/catalina/core/StandardHostValve.java Sun Nov 11 23:26:55
2012
@@ -145,6 +145,9 @@ final class StandardHostValve extends Va
         // If a request init listener throws an exception, the request is
         // aborted
         boolean asyncAtStart = request.isAsync();
+        // 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();
         if (asyncAtStart || context.fireRequestInitEvent(request)) {
 
             // Ask this Context to process this request
@@ -152,29 +155,37 @@ final class StandardHostValve extends Va
                 context.getPipeline().getFirst().invoke(request, response);
             } catch (Throwable t) {
                 ExceptionUtils.handleThrowable(t);
-                request.setAttribute(RequestDispatcher.ERROR_EXCEPTION, t);
-                throwable(request, response, t);
+                if (errorAtStart) {
+                    container.getLogger().error("Exception Processing " +
+                            request.getRequestURI(), t);
+                } else {
+                    request.setAttribute(RequestDispatcher.ERROR_EXCEPTION, t);
+                    throwable(request, response, t);
+                }
             }
 
             // If the request was async at the start and an error occurred then
             // the async error handling will kick-in and that will fire the
             // request destroyed event *after* the error handling has taken
             // place
-            if (!(request.isAsync() || (asyncAtStart && request.getAttribute(
-                        RequestDispatcher.ERROR_EXCEPTION) != null))) {
-                // Protect against NPEs if context was destroyed during a long
-                // running request.
+            if (!(request.isAsync() || (asyncAtStart &&
+                    request.getAttribute(
+                            RequestDispatcher.ERROR_EXCEPTION) != null))) {
+                // Protect against NPEs if context was destroyed during a
+                // long running request.
                 if (context.getState().isAvailable()) {
-                    // Error page processing
-                    response.setSuspended(false);
-
-                    Throwable t = (Throwable) request.getAttribute(
-                            RequestDispatcher.ERROR_EXCEPTION);
-
-                    if (t != null) {
-                        throwable(request, response, t);
-                    } else {
-                        status(request, response);
+                    if (!errorAtStart) {
+                        // Error page processing
+                        response.setSuspended(false);
+
+                        Throwable t = (Throwable) request.getAttribute(
+                                RequestDispatcher.ERROR_EXCEPTION);
+
+                        if (t != null) {
+                            throwable(request, response, t);
+                        } else {
+                            status(request, response);
+                        }
                     }
 
                     context.fireRequestDestroyEvent(request);
@@ -334,7 +345,7 @@ final class StandardHostValve extends Va
      * @param throwable The exception that occurred (which possibly wraps
      *  a root cause exception
      */
-    private void throwable(Request request, Response response,
+    protected void throwable(Request request, Response response,
                              Throwable throwable) {
         Context context = request.getContext();
         if (context == null) {

Modified: tomcat/trunk/java/org/apache/coyote/AsyncStateMachine.java
URL: http://svn.apache.org/viewvc/tomcat/trunk/java/org/apache/coyote/AsyncStateMachine.java?rev=1408148&r1=1408147&r2=1408148&view=diff
==============================================================================
--- tomcat/trunk/java/org/apache/coyote/AsyncStateMachine.java (original)
+++ tomcat/trunk/java/org/apache/coyote/AsyncStateMachine.java Sun Nov 11 23:26:55 2012
@@ -271,7 +271,8 @@ public class AsyncStateMachine<S> {
         if (state == AsyncState.STARTING) {
             state = AsyncState.MUST_DISPATCH;
         } else if (state == AsyncState.STARTED ||
-                state == AsyncState.TIMING_OUT) {
+                state == AsyncState.TIMING_OUT ||
+                state == AsyncState.ERROR) {
             state = AsyncState.DISPATCHING;
             doDispatch = true;
         } else {

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=1408148&r1=1408147&r2=1408148&view=diff
==============================================================================
--- tomcat/trunk/test/org/apache/catalina/core/TestAsyncContextImpl.java (original)
+++ tomcat/trunk/test/org/apache/catalina/core/TestAsyncContextImpl.java Sun Nov 11 23:26:55
2012
@@ -19,6 +19,7 @@ package org.apache.catalina.core;
 
 import java.io.File;
 import java.io.IOException;
+import java.io.PrintWriter;
 import java.util.LinkedHashMap;
 import java.util.List;
 import java.util.Map;
@@ -38,6 +39,8 @@ import javax.servlet.http.HttpServlet;
 import javax.servlet.http.HttpServletRequest;
 import javax.servlet.http.HttpServletResponse;
 
+import junit.framework.Assert;
+
 import static org.junit.Assert.assertEquals;
 import static org.junit.Assert.assertNotNull;
 import static org.junit.Assert.assertTrue;
@@ -48,6 +51,7 @@ import org.junit.Test;
 import org.apache.catalina.Context;
 import org.apache.catalina.Wrapper;
 import org.apache.catalina.connector.Request;
+import org.apache.catalina.deploy.ErrorPage;
 import org.apache.catalina.startup.Tomcat;
 import org.apache.catalina.startup.TomcatBaseTest;
 import org.apache.catalina.valves.TesterAccessLogValve;
@@ -174,7 +178,7 @@ public class TestAsyncContextImpl extend
         assertEquals("OK-run2", bc2.toString());
 
         // Check the access log
-        alv.validateAccessLog(2, 200,
+        alv.validateAccessLog(2, 500,
                 AsyncStartNoCompleteServlet.ASYNC_TIMEOUT,
                 AsyncStartNoCompleteServlet.ASYNC_TIMEOUT + TIMEOUT_MARGIN +
                         REQUEST_TIME);
@@ -380,29 +384,34 @@ public class TestAsyncContextImpl extend
     @Test
     public void testTimeoutListenerCompleteNoDispatch() throws Exception {
         // Should work
-        doTestTimeout(true, null);
+        doTestTimeout(Boolean.TRUE, null);
     }
 
     @Test
     public void testTimeoutListenerNoCompleteNoDispatch() throws Exception {
         // Should trigger an error - must do one or other
-        doTestTimeout(false, null);
+        doTestTimeout(Boolean.FALSE, null);
     }
 
     @Test
     public void testTimeoutListenerCompleteDispatch() throws Exception {
         // Should trigger an error - can't do both
-        doTestTimeout(true, "/nonasync");
+        doTestTimeout(Boolean.TRUE, "/nonasync");
     }
 
     @Test
     public void testTimeoutListenerNoCompleteDispatch() throws Exception {
         // Should work
-        doTestTimeout(false, "/nonasync");
+        doTestTimeout(Boolean.FALSE, "/nonasync");
     }
 
+    @Test
+    public void testTimeoutNoListener() throws Exception {
+        // Should work
+        doTestTimeout(null, null);
+    }
 
-    private void doTestTimeout(boolean completeOnTimeout, String dispatchUrl)
+    private void doTestTimeout(Boolean completeOnTimeout, String dispatchUrl)
     throws Exception {
         // Setup Tomcat instance
         Tomcat tomcat = getTomcatInstance();
@@ -420,7 +429,7 @@ public class TestAsyncContextImpl extend
         Context ctx = tomcat.addContext("", docBase.getAbsolutePath());
 
         TimeoutServlet timeout =
-            new TimeoutServlet(completeOnTimeout, dispatchUrl);
+                new TimeoutServlet(completeOnTimeout, dispatchUrl);
 
         Wrapper wrapper = Tomcat.addServlet(ctx, "time", timeout);
         wrapper.setAsyncSupported(true);
@@ -447,8 +456,11 @@ public class TestAsyncContextImpl extend
             // Ignore - expected for some error conditions
         }
         StringBuilder expected = new StringBuilder("requestInitialized-");
-        expected.append("TimeoutServletGet-onTimeout-");
-        if (completeOnTimeout) {
+        expected.append("TimeoutServletGet-");
+        if (completeOnTimeout == null) {
+            expected.append("requestDestroyed");
+        } else if (completeOnTimeout.booleanValue()) {
+            expected.append("onTimeout-");
             if (dispatchUrl == null) {
                 expected.append("onComplete-");
                 expected.append("requestDestroyed");
@@ -459,6 +471,7 @@ public class TestAsyncContextImpl extend
                 // never happens.
             }
         } else {
+            expected.append("onTimeout-");
             if (dispatchUrl == null) {
                 expected.append("onError-");
             } else {
@@ -470,7 +483,14 @@ public class TestAsyncContextImpl extend
         assertEquals(expected.toString(), res.toString());
 
         // Check the access log
-        if (completeOnTimeout && dispatchUrl != null) {
+        if (completeOnTimeout == null) {
+            alvGlobal.validateAccessLog(1, 500, TimeoutServlet.ASYNC_TIMEOUT,
+                    TimeoutServlet.ASYNC_TIMEOUT + TIMEOUT_MARGIN +
+                    REQUEST_TIME);
+            alv.validateAccessLog(1, 500, TimeoutServlet.ASYNC_TIMEOUT,
+                    TimeoutServlet.ASYNC_TIMEOUT + TIMEOUT_MARGIN +
+                    REQUEST_TIME);
+        } else if (completeOnTimeout.booleanValue() && dispatchUrl != null) {
             // This error is written into Host-level AccessLogValve only
             alvGlobal.validateAccessLog(1, 500, 0, TimeoutServlet.ASYNC_TIMEOUT
                     + TIMEOUT_MARGIN + REQUEST_TIME);
@@ -488,12 +508,12 @@ public class TestAsyncContextImpl extend
     private static class TimeoutServlet extends HttpServlet {
         private static final long serialVersionUID = 1L;
 
-        private final boolean completeOnTimeout;
+        private final Boolean completeOnTimeout;
         private final String dispatchUrl;
 
         public static final long ASYNC_TIMEOUT = 3000;
 
-        public TimeoutServlet(boolean completeOnTimeout, String dispatchUrl) {
+        public TimeoutServlet(Boolean completeOnTimeout, String dispatchUrl) {
             this.completeOnTimeout = completeOnTimeout;
             this.dispatchUrl = dispatchUrl;
         }
@@ -506,8 +526,10 @@ public class TestAsyncContextImpl extend
                 final AsyncContext ac = req.startAsync();
                 ac.setTimeout(ASYNC_TIMEOUT);
 
-                ac.addListener(new TrackingListener(
-                        false, completeOnTimeout, dispatchUrl));
+                if (completeOnTimeout != null) {
+                    ac.addListener(new TrackingListener(false,
+                            completeOnTimeout.booleanValue(), dispatchUrl));
+                }
             } else {
                 resp.getWriter().print("FAIL: Async unsupported");
             }
@@ -672,7 +694,7 @@ public class TestAsyncContextImpl extend
         wrapper.setAsyncSupported(true);
         ctx.addServletMapping("/stage1", "tracking");
 
-        TimeoutServlet timeout = new TimeoutServlet(true, null);
+        TimeoutServlet timeout = new TimeoutServlet(Boolean.TRUE, null);
         Wrapper wrapper2 = Tomcat.addServlet(ctx, "timeout", timeout);
         wrapper2.setAsyncSupported(true);
         ctx.addServletMapping("/stage2", "timeout");
@@ -1453,4 +1475,160 @@ public class TestAsyncContextImpl extend
             resp.getWriter().print("OK");
         }
     }
+
+    @Test
+    public void testTimeoutErrorDispatchNone() throws Exception {
+        doTestTimeoutErrorDispatch(null, null);
+    }
+
+    @Test
+    public void testTimeoutErrorDispatchNonAsync() throws Exception {
+        doTestTimeoutErrorDispatch(Boolean.FALSE, null);
+    }
+
+    @Test
+    public void testTimeoutErrorDispatchAsyncStart() throws Exception {
+        doTestTimeoutErrorDispatch(
+                Boolean.TRUE, ErrorPageAsyncMode.NO_COMPLETE);
+    }
+
+    @Test
+    public void testTimeoutErrorDispatchAsyncComplete() throws Exception {
+        doTestTimeoutErrorDispatch(Boolean.TRUE, ErrorPageAsyncMode.COMPLETE);
+    }
+
+    @Test
+    public void testTimeoutErrorDispatchAsyncDispatch() throws Exception {
+        doTestTimeoutErrorDispatch(Boolean.TRUE, ErrorPageAsyncMode.DISPATCH);
+    }
+
+    private void doTestTimeoutErrorDispatch(Boolean asyncError,
+            ErrorPageAsyncMode mode) throws Exception {
+        // Setup Tomcat instance
+        Tomcat tomcat = getTomcatInstance();
+
+        // Must have a real docBase - just use temp
+        File docBase = new File(System.getProperty("java.io.tmpdir"));
+
+        Context ctx = tomcat.addContext("", docBase.getAbsolutePath());
+
+        TimeoutServlet timeout = new TimeoutServlet(null, null);
+        Wrapper w1 = Tomcat.addServlet(ctx, "time", timeout);
+        w1.setAsyncSupported(true);
+        ctx.addServletMapping("/async", "time");
+
+        NonAsyncServlet nonAsync = new NonAsyncServlet();
+        Wrapper w2 = Tomcat.addServlet(ctx, "nonAsync", nonAsync);
+        w2.setAsyncSupported(true);
+        ctx.addServletMapping("/error/nonasync", "nonAsync");
+
+        AsyncErrorPage asyncErrorPage = new AsyncErrorPage(mode);
+        Wrapper w3 = Tomcat.addServlet(ctx, "asyncErrorPage", asyncErrorPage);
+        w3.setAsyncSupported(true);
+        ctx.addServletMapping("/error/async", "asyncErrorPage");
+
+        if (asyncError != null) {
+            ErrorPage ep = new ErrorPage();
+            ep.setErrorCode(500);
+            if (asyncError.booleanValue()) {
+                ep.setLocation("/error/async");
+            } else {
+                ep.setLocation("/error/nonasync");
+            }
+
+            ctx.addErrorPage(ep);
+        }
+
+        ctx.addApplicationListener(TrackingRequestListener.class.getName());
+
+        TesterAccessLogValve alv = new TesterAccessLogValve();
+        ctx.getPipeline().addValve(alv);
+        TesterAccessLogValve alvGlobal = new TesterAccessLogValve();
+        tomcat.getHost().getPipeline().addValve(alvGlobal);
+
+        tomcat.start();
+        ByteChunk res = new ByteChunk();
+        try {
+            getUrl("http://localhost:" + getPort() + "/async", res, null);
+        } catch (IOException ioe) {
+            // Ignore - expected for some error conditions
+        }
+
+        StringBuilder expected = new StringBuilder();
+        if (asyncError == null) {
+            // No error handler - just get the 500 response
+            expected.append("requestInitialized-TimeoutServletGet-");
+            // Note: With an error handler the response will be reset and these
+            //       will be lost
+        }
+        if (asyncError != null) {
+            if (asyncError.booleanValue()) {
+                expected.append("AsyncErrorPageGet-");
+                if (mode == ErrorPageAsyncMode.NO_COMPLETE){
+                    expected.append("NoOp-");
+                } else if (mode == ErrorPageAsyncMode.COMPLETE) {
+                    expected.append("Complete-");
+                } else if (mode == ErrorPageAsyncMode.DISPATCH) {
+                    expected.append("Dispatch-NonAsyncServletGet-");
+                }
+            } else {
+                expected.append("NonAsyncServletGet-");
+            }
+        }
+        expected.append("requestDestroyed");
+
+        Assert.assertEquals(expected.toString(), res.toString());
+
+        // Check the access log
+        alvGlobal.validateAccessLog(1, 500, TimeoutServlet.ASYNC_TIMEOUT,
+                TimeoutServlet.ASYNC_TIMEOUT + TIMEOUT_MARGIN +
+                REQUEST_TIME);
+        alv.validateAccessLog(1, 500, TimeoutServlet.ASYNC_TIMEOUT,
+                TimeoutServlet.ASYNC_TIMEOUT + TIMEOUT_MARGIN +
+                REQUEST_TIME);
+    }
+
+    private static enum ErrorPageAsyncMode {
+        NO_COMPLETE,
+        COMPLETE,
+        DISPATCH
+    }
+
+    private static class AsyncErrorPage extends HttpServlet {
+
+        private static final long serialVersionUID = 1L;
+
+        private final ErrorPageAsyncMode mode;
+
+        public AsyncErrorPage(ErrorPageAsyncMode mode) {
+            this.mode = mode;
+        }
+
+        @Override
+        protected void doGet(HttpServletRequest req, HttpServletResponse resp)
+                throws ServletException, IOException {
+            PrintWriter writer = resp.getWriter();
+            writer.write("AsyncErrorPageGet-");
+            resp.flushBuffer();
+
+            final AsyncContext ctxt = req.getAsyncContext();
+
+            switch(mode) {
+                case COMPLETE:
+                    writer.write("Complete-");
+                    ctxt.complete();
+                    break;
+                case DISPATCH:
+                    writer.write("Dispatch-");
+                    ctxt.dispatch("/error/nonasync");
+                    break;
+                case NO_COMPLETE:
+                    writer.write("NoOp-");
+                    break;
+                default:
+                    // Impossible
+                    break;
+            }
+        }
+    }
 }



---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@tomcat.apache.org
For additional commands, e-mail: dev-help@tomcat.apache.org


Mime
View raw message