2012/11/12 <ma...@apache.org>:
> 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);
> }
> }
The spec in 2.3.3.3 says "If none of the listeners called
AsyncContext.complete or any of the AsyncContext.dispatch methods,
then perform an error dispatch...."
As far as I see, the code below is executed unconditionally, but the
spec says that it has to be executed only if the listeners did not do
the calls....
> +
> + // 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
>
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscr...@tomcat.apache.org
For additional commands, e-mail: dev-h...@tomcat.apache.org
