Author: markt Date: Sun Nov 11 23:35:41 2012 New Revision: 1408152 URL: http://svn.apache.org/viewvc?rev=1408152&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/tc7.0.x/trunk/ (props changed) tomcat/tc7.0.x/trunk/java/org/apache/catalina/core/AsyncContextImpl.java tomcat/tc7.0.x/trunk/java/org/apache/catalina/core/StandardHostValve.java tomcat/tc7.0.x/trunk/java/org/apache/coyote/AsyncStateMachine.java tomcat/tc7.0.x/trunk/test/org/apache/catalina/core/TestAsyncContextImpl.java tomcat/tc7.0.x/trunk/webapps/docs/changelog.xml Propchange: tomcat/tc7.0.x/trunk/ ------------------------------------------------------------------------------ Merged /tomcat/trunk:r1408148 Modified: tomcat/tc7.0.x/trunk/java/org/apache/catalina/core/AsyncContextImpl.java URL: http://svn.apache.org/viewvc/tomcat/tc7.0.x/trunk/java/org/apache/catalina/core/AsyncContextImpl.java?rev=1408152&r1=1408151&r2=1408152&view=diff ============================================================================== --- tomcat/tc7.0.x/trunk/java/org/apache/catalina/core/AsyncContextImpl.java (original) +++ tomcat/tc7.0.x/trunk/java/org/apache/catalina/core/AsyncContextImpl.java Sun Nov 11 23:35:41 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; @@ -128,8 +130,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 { @@ -372,6 +374,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/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=1408152&r1=1408151&r2=1408152&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 Sun Nov 11 23:35:41 2012 @@ -161,6 +161,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 @@ -168,29 +171,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); + if (!errorAtStart) { + // Error page processing + response.setSuspended(false); - Throwable t = (Throwable) request.getAttribute( - RequestDispatcher.ERROR_EXCEPTION); + Throwable t = (Throwable) request.getAttribute( + RequestDispatcher.ERROR_EXCEPTION); - if (t != null) { - throwable(request, response, t); - } else { - status(request, response); + if (t != null) { + throwable(request, response, t); + } else { + status(request, response); + } } context.fireRequestDestroyEvent(request); @@ -348,7 +359,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/tc7.0.x/trunk/java/org/apache/coyote/AsyncStateMachine.java URL: http://svn.apache.org/viewvc/tomcat/tc7.0.x/trunk/java/org/apache/coyote/AsyncStateMachine.java?rev=1408152&r1=1408151&r2=1408152&view=diff ============================================================================== --- tomcat/tc7.0.x/trunk/java/org/apache/coyote/AsyncStateMachine.java (original) +++ tomcat/tc7.0.x/trunk/java/org/apache/coyote/AsyncStateMachine.java Sun Nov 11 23:35:41 2012 @@ -249,7 +249,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/tc7.0.x/trunk/test/org/apache/catalina/core/TestAsyncContextImpl.java URL: http://svn.apache.org/viewvc/tomcat/tc7.0.x/trunk/test/org/apache/catalina/core/TestAsyncContextImpl.java?rev=1408152&r1=1408151&r2=1408152&view=diff ============================================================================== --- tomcat/tc7.0.x/trunk/test/org/apache/catalina/core/TestAsyncContextImpl.java (original) +++ tomcat/tc7.0.x/trunk/test/org/apache/catalina/core/TestAsyncContextImpl.java Sun Nov 11 23:35:41 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"); @@ -1454,4 +1476,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; + } + } + } } 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=1408152&r1=1408151&r2=1408152&view=diff ============================================================================== --- tomcat/tc7.0.x/trunk/webapps/docs/changelog.xml (original) +++ tomcat/tc7.0.x/trunk/webapps/docs/changelog.xml Sun Nov 11 23:35:41 2012 @@ -123,6 +123,11 @@ <code>AsyncListener.complete()</code> are called with the correct thread context class loader. (fhanik) </fix> + <fix> + <bug>54123</bug>: If an asynchronous request times out without any + <code>AsyncListener</code>s defined, a 500 error will be triggered. + (markt) + </fix> </changelog> </subsection> <subsection name="Coyote"> --------------------------------------------------------------------- To unsubscribe, e-mail: dev-unsubscr...@tomcat.apache.org For additional commands, e-mail: dev-h...@tomcat.apache.org