From 8cfcc8be227ca569817770b47b559a4743a5f912 Mon Sep 17 00:00:00 2001 From: markt Date: Wed, 16 Feb 2011 17:35:24 +0000 Subject: [PATCH] Fix https://issues.apache.org/bugzilla/show_bug.cgi?id=50793 Correctly fire request init/destroy events for astnc requests git-svn-id: https://svn.apache.org/repos/asf/tomcat/trunk@1071321 13f79535-47bb-0310-9956-ffa450edef68 --- .../apache/catalina/connector/CoyoteAdapter.java | 11 +++ java/org/apache/catalina/core/StandardContext.java | 87 ++++++++++------------ .../apache/catalina/core/StandardContextValve.java | 14 +++- .../apache/catalina/core/TestAsyncContextImpl.java | 47 +++++++++++- webapps/docs/changelog.xml | 5 ++ 5 files changed, 110 insertions(+), 54 deletions(-) diff --git a/java/org/apache/catalina/connector/CoyoteAdapter.java b/java/org/apache/catalina/connector/CoyoteAdapter.java index 153788f91..9d2e2b0c9 100644 --- a/java/org/apache/catalina/connector/CoyoteAdapter.java +++ b/java/org/apache/catalina/connector/CoyoteAdapter.java @@ -268,6 +268,17 @@ public class CoyoteAdapter implements Adapter { boolean success = true; AsyncContextImpl asyncConImpl = (AsyncContextImpl)request.getAsyncContext(); try { + if (!request.isAsync() && !comet) { + // Error or timeout - need to tell listeners the request is over + // Have to test this first since state may change while in this + // method and this is only required if entering this methos in + // this state + Context ctxt = (Context) request.getMappingData().context; + if (ctxt != null) { + ctxt.fireRequestDestroyEvent(request); + } + } + if (status==SocketStatus.TIMEOUT) { success = true; if (!asyncConImpl.timeout()) { diff --git a/java/org/apache/catalina/core/StandardContext.java b/java/org/apache/catalina/core/StandardContext.java index c7bc03bb5..c84889a09 100644 --- a/java/org/apache/catalina/core/StandardContext.java +++ b/java/org/apache/catalina/core/StandardContext.java @@ -50,7 +50,6 @@ import javax.management.NotificationListener; import javax.management.ObjectName; import javax.naming.NamingException; import javax.naming.directory.DirContext; -import javax.servlet.DispatcherType; import javax.servlet.FilterConfig; import javax.servlet.RequestDispatcher; import javax.servlet.Servlet; @@ -5817,30 +5816,26 @@ public class StandardContext extends ContainerBase if ((instances != null) && (instances.length > 0)) { - // Don't fire the listener for async requests - if (!DispatcherType.ASYNC.equals(request.getDispatcherType())) { + ServletRequestEvent event = + new ServletRequestEvent(getServletContext(), request); + + for (int i = 0; i < instances.length; i++) { + if (instances[i] == null) + continue; + if (!(instances[i] instanceof ServletRequestListener)) + continue; + ServletRequestListener listener = + (ServletRequestListener) instances[i]; - ServletRequestEvent event = - new ServletRequestEvent(getServletContext(), request); - - for (int i = 0; i < instances.length; i++) { - if (instances[i] == null) - continue; - if (!(instances[i] instanceof ServletRequestListener)) - continue; - ServletRequestListener listener = - (ServletRequestListener) instances[i]; - - try { - listener.requestInitialized(event); - } catch (Throwable t) { - ExceptionUtils.handleThrowable(t); - getLogger().error(sm.getString( - "standardContext.requestListener.requestInit", - instances[i].getClass().getName()), t); - request.setAttribute(RequestDispatcher.ERROR_EXCEPTION, t); - return false; - } + try { + listener.requestInitialized(event); + } catch (Throwable t) { + ExceptionUtils.handleThrowable(t); + getLogger().error(sm.getString( + "standardContext.requestListener.requestInit", + instances[i].getClass().getName()), t); + request.setAttribute(RequestDispatcher.ERROR_EXCEPTION, t); + return false; } } } @@ -5854,31 +5849,27 @@ public class StandardContext extends ContainerBase if ((instances != null) && (instances.length > 0)) { - // Don't fire the listener for async requests - if (!DispatcherType.ASYNC.equals(request.getDispatcherType())) { + ServletRequestEvent event = + new ServletRequestEvent(getServletContext(), request); - ServletRequestEvent event = - new ServletRequestEvent(getServletContext(), request); - - for (int i = 0; i < instances.length; i++) { - int j = (instances.length -1) -i; - if (instances[j] == null) - continue; - if (!(instances[j] instanceof ServletRequestListener)) - continue; - ServletRequestListener listener = - (ServletRequestListener) instances[j]; - - try { - listener.requestDestroyed(event); - } catch (Throwable t) { - ExceptionUtils.handleThrowable(t); - getLogger().error(sm.getString( - "standardContext.requestListener.requestInit", - instances[j].getClass().getName()), t); - request.setAttribute(RequestDispatcher.ERROR_EXCEPTION, t); - return false; - } + for (int i = 0; i < instances.length; i++) { + int j = (instances.length -1) -i; + if (instances[j] == null) + continue; + if (!(instances[j] instanceof ServletRequestListener)) + continue; + ServletRequestListener listener = + (ServletRequestListener) instances[j]; + + try { + listener.requestDestroyed(event); + } catch (Throwable t) { + ExceptionUtils.handleThrowable(t); + getLogger().error(sm.getString( + "standardContext.requestListener.requestInit", + instances[j].getClass().getName()), t); + request.setAttribute(RequestDispatcher.ERROR_EXCEPTION, t); + return false; } } } diff --git a/java/org/apache/catalina/core/StandardContextValve.java b/java/org/apache/catalina/core/StandardContextValve.java index 1a29ee014..2b4599763 100644 --- a/java/org/apache/catalina/core/StandardContextValve.java +++ b/java/org/apache/catalina/core/StandardContextValve.java @@ -21,6 +21,7 @@ package org.apache.catalina.core; import java.io.IOException; +import javax.servlet.RequestDispatcher; import javax.servlet.ServletException; import javax.servlet.http.HttpServletResponse; @@ -152,15 +153,24 @@ final class StandardContextValve } } + // Don't fire listeners during async processing // If a request init listener throws an exception, the request is // aborted - if (context.fireRequestInitEvent(request)) { + boolean asyncAtStart = request.isAsync(); + if (asyncAtStart || context.fireRequestInitEvent(request)) { if (request.isAsyncSupported()) { request.setAsyncSupported(wrapper.getPipeline().isAsyncSupported()); } wrapper.getPipeline().getFirst().invoke(request, response); - context.fireRequestDestroyEvent(request); + // 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))) { + context.fireRequestDestroyEvent(request); + } } } diff --git a/test/org/apache/catalina/core/TestAsyncContextImpl.java b/test/org/apache/catalina/core/TestAsyncContextImpl.java index fcd0d0287..0bd49eac0 100644 --- a/test/org/apache/catalina/core/TestAsyncContextImpl.java +++ b/test/org/apache/catalina/core/TestAsyncContextImpl.java @@ -27,6 +27,8 @@ import javax.servlet.AsyncContext; import javax.servlet.AsyncEvent; import javax.servlet.AsyncListener; import javax.servlet.ServletException; +import javax.servlet.ServletRequestEvent; +import javax.servlet.ServletRequestListener; import javax.servlet.ServletResponse; import javax.servlet.http.HttpServlet; import javax.servlet.http.HttpServletRequest; @@ -34,6 +36,7 @@ import javax.servlet.http.HttpServletResponse; import org.apache.catalina.Context; import org.apache.catalina.Wrapper; +import org.apache.catalina.connector.Request; import org.apache.catalina.startup.Tomcat; import org.apache.catalina.startup.TomcatBaseTest; import org.apache.tomcat.util.buf.ByteChunk; @@ -358,9 +361,11 @@ public class TestAsyncContextImpl extends TomcatBaseTest { ctx.addServletMapping(dispatchUrl, "nonasync"); } + ctx.addApplicationListener(TrackingRequestListener.class.getName()); + tomcat.start(); ByteChunk res = getUrl("http://localhost:" + getPort() + "/async"); - StringBuilder expected = new StringBuilder(); + StringBuilder expected = new StringBuilder("requestInitialized-"); expected.append("TimeoutServletGet-onTimeout-"); if (!completeOnTimeout) { expected.append("onError-"); @@ -370,6 +375,7 @@ public class TestAsyncContextImpl extends TomcatBaseTest { } else { expected.append("NonAsyncServletGet-"); } + expected.append("requestDestroyed"); assertEquals(expected.toString(), res.toString()); } @@ -442,6 +448,8 @@ public class TestAsyncContextImpl extends TomcatBaseTest { wrapper2.setAsyncSupported(true); ctx.addServletMapping("/stage2", "nonasync"); + ctx.addApplicationListener(TrackingRequestListener.class.getName()); + tomcat.start(); StringBuilder url = new StringBuilder(48); @@ -454,13 +462,14 @@ public class TestAsyncContextImpl extends TomcatBaseTest { } ByteChunk res = getUrl(url.toString()); - StringBuilder expected = new StringBuilder(); + StringBuilder expected = new StringBuilder("requestInitialized-"); int loop = iter; while (loop > 0) { expected.append("DispatchingServletGet-"); loop--; } expected.append("NonAsyncServletGet-"); + expected.append("requestDestroyed"); assertEquals(expected.toString(), res.toString()); } @@ -643,6 +652,34 @@ public class TestAsyncContextImpl extends TomcatBaseTest { } } + public static class TrackingRequestListener + implements ServletRequestListener { + + @Override + public void requestDestroyed(ServletRequestEvent sre) { + // Need the response and it isn't available via the Servlet API + Request r = (Request) sre.getServletRequest(); + try { + r.getResponse().getWriter().print("requestDestroyed"); + } catch (IOException e) { + // Test will fail if this happens + e.printStackTrace(); + } + } + + @Override + public void requestInitialized(ServletRequestEvent sre) { + // Need the response and it isn't available via the Servlet API + Request r = (Request) sre.getServletRequest(); + try { + r.getResponse().getWriter().print("requestInitialized-"); + } catch (IOException e) { + // Test will fail if this happens + e.printStackTrace(); + } + } + } + public void testDispatchErrorSingle() throws Exception { doTestDispatchError(1, false, false); } @@ -715,6 +752,8 @@ public class TestAsyncContextImpl extends TomcatBaseTest { Tomcat.addServlet(ctx, "error", error); ctx.addServletMapping("/stage2", "error"); + ctx.addApplicationListener(TrackingRequestListener.class.getName()); + tomcat.start(); StringBuilder url = new StringBuilder(48); @@ -727,7 +766,7 @@ public class TestAsyncContextImpl extends TomcatBaseTest { } ByteChunk res = getUrl(url.toString()); - StringBuilder expected = new StringBuilder(); + StringBuilder expected = new StringBuilder("requestInitialized-"); int loop = iter; while (loop > 0) { expected.append("DispatchingServletGet-"); @@ -736,7 +775,7 @@ public class TestAsyncContextImpl extends TomcatBaseTest { } loop--; } - expected.append("ErrorServletGet-onError-onComplete-"); + expected.append("ErrorServletGet-onError-onComplete-requestDestroyed"); assertEquals(expected.toString(), res.toString()); } diff --git a/webapps/docs/changelog.xml b/webapps/docs/changelog.xml index 0b5047074..75f1dec55 100644 --- a/webapps/docs/changelog.xml +++ b/webapps/docs/changelog.xml @@ -88,6 +88,11 @@ 50752: Fix typo in debug message in deprecated Embedded class. (markt) + + 50793: When processing Servlet 3.0 async requests, ensure + that the requestInitialized and requestDestroyed events are only fired + once per request at the correct times. (markt) + -- 2.11.0