You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@tomcat.apache.org by ma...@apache.org on 2012/11/12 00:26:58 UTC
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
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