Author: markt
Date: Thu Jul 22 09:12:18 2010
New Revision: 966548
URL: http://svn.apache.org/viewvc?rev=966548&view=rev
Log:
Fix https://issues.apache.org/bugzilla/show_bug.cgi?id=49567
Better handling of calls to complete() from a separate thread.
There are still a handful of TODOs around this which will follow shortly but
I'm committing this since it passes the unit tests and the Servlet TCK (with
BIO)
Modified:
tomcat/trunk/java/org/apache/catalina/connector/CoyoteAdapter.java
tomcat/trunk/java/org/apache/catalina/connector/Request.java
tomcat/trunk/java/org/apache/catalina/core/AsyncContextImpl.java
tomcat/trunk/java/org/apache/coyote/http11/Http11Processor.java
tomcat/trunk/test/org/apache/catalina/core/TestAsyncContextImpl.java
tomcat/trunk/webapps/docs/changelog.xml
Modified: tomcat/trunk/java/org/apache/catalina/connector/CoyoteAdapter.java
URL:
http://svn.apache.org/viewvc/tomcat/trunk/java/org/apache/catalina/connector/CoyoteAdapter.java?rev=966548&r1=966547&r2=966548&view=diff
==============================================================================
--- tomcat/trunk/java/org/apache/catalina/connector/CoyoteAdapter.java
(original)
+++ tomcat/trunk/java/org/apache/catalina/connector/CoyoteAdapter.java Thu Jul
22 09:12:18 2010
@@ -267,7 +267,7 @@ public class CoyoteAdapter implements Ad
//configure settings for timed out
asyncConImpl.setTimeoutState();
}
- if (status==SocketStatus.ERROR || status==SocketStatus.STOP ||
status==SocketStatus.DISCONNECT) {
+ if (status==SocketStatus.ERROR || status==SocketStatus.DISCONNECT)
{
AsyncContextImpl asyncConImpl =
(AsyncContextImpl)request.getAsyncContext();
//TODO SERVLET3 - async
//configure settings for timed out
Modified: tomcat/trunk/java/org/apache/catalina/connector/Request.java
URL:
http://svn.apache.org/viewvc/tomcat/trunk/java/org/apache/catalina/connector/Request.java?rev=966548&r1=966547&r2=966548&view=diff
==============================================================================
--- tomcat/trunk/java/org/apache/catalina/connector/Request.java (original)
+++ tomcat/trunk/java/org/apache/catalina/connector/Request.java Thu Jul 22
09:12:18 2010
@@ -1582,7 +1582,6 @@ public class Request
}
return
(asyncContext.getState()==AsyncContextImpl.AsyncState.DISPATCHING ||
-
asyncContext.getState()==AsyncContextImpl.AsyncState.DISPATCHING_RUNNABLE ||
asyncContext.getState()==AsyncContextImpl.AsyncState.TIMING_OUT ||
asyncContext.getState()==AsyncContextImpl.AsyncState.STARTED
||
asyncContext.getState()==AsyncContextImpl.AsyncState.ERROR_DISPATCHING ||
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=966548&r1=966547&r2=966548&view=diff
==============================================================================
--- tomcat/trunk/java/org/apache/catalina/core/AsyncContextImpl.java (original)
+++ tomcat/trunk/java/org/apache/catalina/core/AsyncContextImpl.java Thu Jul 22
09:12:18 2010
@@ -48,8 +48,8 @@ import org.apache.juli.logging.LogFactor
public class AsyncContextImpl implements AsyncContext {
public static enum AsyncState {
- NOT_STARTED, STARTED, DISPATCHING, DISPATCHING_RUNNABLE, DISPATCHED,
- COMPLETING, COMPLETING_RUNNABLE, TIMING_OUT, ERROR_DISPATCHING
+ NOT_STARTED, STARTED, DISPATCHING, DISPATCHED, COMPLETING, TIMING_OUT,
+ ERROR_DISPATCHING
}
private static final Log log = LogFactory.getLog(AsyncContextImpl.class);
@@ -87,9 +87,6 @@ public class AsyncContextImpl implements
AtomicBoolean dispatched = new AtomicBoolean(false);
request.getCoyoteRequest().action(ActionCode.ACTION_ASYNC_COMPLETE,dispatched);
if (!dispatched.get()) doInternalComplete(false);
- } else if (state.compareAndSet(AsyncState.DISPATCHING_RUNNABLE,
- AsyncState.COMPLETING_RUNNABLE)) {
- // do nothing
} else {
throw new IllegalStateException("Complete not allowed. Invalid
state:"+state.get());
}
@@ -182,33 +179,16 @@ public class AsyncContextImpl implements
log.debug("AsyncContext Start Called["+state.get()+";
"+request.getRequestURI()+"?"+request.getQueryString()+"]", new
DebugException());
}
- if (state.compareAndSet(AsyncState.STARTED,
AsyncState.DISPATCHING_RUNNABLE) ||
- state.compareAndSet(AsyncState.DISPATCHED,
AsyncState.DISPATCHING_RUNNABLE)) {
- // TODO SERVLET3 - async
- final ServletContext sctx =
getServletRequest().getServletContext();
- Runnable r = new Runnable() {
- public void run() {
- //TODO SERVLET3 - async - set context class loader when
running the task.
- try {
-
- run.run();
- }catch (Exception x) {
- log.error("Unable to run async task.",x);
- }
- }
- };
- this.dispatch = r;
- AtomicBoolean dispatched = new AtomicBoolean(false);
-
request.getCoyoteRequest().action(ActionCode.ACTION_ASYNC_DISPATCH, dispatched
);
- if (!dispatched.get()) {
- try {
- doInternalDispatch();
- }catch (ServletException sx) {
- throw new RuntimeException(sx);
- }catch (IOException ix) {
- throw new RuntimeException(ix);
- }
- }
+ if (state.get() == AsyncState.STARTED) {
+ // TODO SERVLET3 - async - set context class loader when running
the
+ // task.
+ // final ServletContext sctx =
getServletRequest().getServletContext();
+ // TODO - Use a container thread without creating a memory leak
+ // Execute the runnable using a container thread from the
+ // Connector's thread pool
+ //
request.getConnector().getProtocolHandler().getExecutor().execute(run);
+ Thread t = new Thread(run);
+ t.start();
} else {
throw new IllegalStateException("Dispatch not allowed. Invalid
state:"+state.get());
}
@@ -258,8 +238,7 @@ public class AsyncContextImpl implements
public boolean isStarted() {
return (state.get() == AsyncState.STARTED ||
- state.get() == AsyncState.DISPATCHING ||
- state.get() == AsyncState.DISPATCHING_RUNNABLE);
+ state.get() == AsyncState.DISPATCHING);
}
public void setStarted(Context context) {
@@ -304,10 +283,16 @@ public class AsyncContextImpl implements
listener.fireOnTimeout(event);
listenerInvoked = true;
}
- if (!listenerInvoked) {
- ((HttpServletResponse)servletResponse).setStatus(500);
+ if (listenerInvoked) {
+ // Listener should have called complete
+ if (state.get() != AsyncState.NOT_STARTED) {
+ ((HttpServletResponse)servletResponse).setStatus(500);
+ doInternalComplete(true);
+ }
+ } else {
+ // No listeners, container calls complete
+ doInternalComplete(false);
}
- doInternalComplete(true);
} else if (this.state.compareAndSet(AsyncState.ERROR_DISPATCHING,
AsyncState.COMPLETING)) {
log.debug("ON ERROR!");
boolean listenerInvoked = false;
@@ -339,27 +324,6 @@ public class AsyncContextImpl implements
dispatch = null;
}
}
- } else if (this.state.get() == AsyncState.DISPATCHING_RUNNABLE) {
- if (this.dispatch!=null) {
- try {
- dispatch.run();
- } catch (RuntimeException x) {
- doInternalComplete(true);
- if (x.getCause() instanceof ServletException) throw
(ServletException)x.getCause();
- if (x.getCause() instanceof IOException) throw
(IOException)x.getCause();
- else throw new ServletException(x);
- } finally {
- dispatch = null;
- }
- if (this.state.compareAndSet(AsyncState.COMPLETING_RUNNABLE,
- AsyncState.COMPLETING)) {
- doInternalComplete(false);
- } else if (this.state.get() ==
AsyncState.DISPATCHING_RUNNABLE) {
- doInternalComplete(true);
- throw new IllegalStateException(
- "Failed to call dispatch() or complete() after
start()");
- }
- }
} else if (this.state.get()==AsyncState.COMPLETING) {
doInternalComplete(false);
} else {
Modified: tomcat/trunk/java/org/apache/coyote/http11/Http11Processor.java
URL:
http://svn.apache.org/viewvc/tomcat/trunk/java/org/apache/coyote/http11/Http11Processor.java?rev=966548&r1=966547&r2=966548&view=diff
==============================================================================
--- tomcat/trunk/java/org/apache/coyote/http11/Http11Processor.java (original)
+++ tomcat/trunk/java/org/apache/coyote/http11/Http11Processor.java Thu Jul 22
09:12:18 2010
@@ -349,8 +349,8 @@ public class Http11Processor extends Abs
} else if (async) {
return SocketState.LONG;
} else {
+ recycle();
if (!keepAlive) {
- recycle();
return SocketState.CLOSED;
} else {
return SocketState.OPEN;
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=966548&r1=966547&r2=966548&view=diff
==============================================================================
--- tomcat/trunk/test/org/apache/catalina/core/TestAsyncContextImpl.java
(original)
+++ tomcat/trunk/test/org/apache/catalina/core/TestAsyncContextImpl.java Thu
Jul 22 09:12:18 2010
@@ -19,6 +19,7 @@ package org.apache.catalina.core;
import java.io.IOException;
+import javax.servlet.AsyncContext;
import javax.servlet.ServletException;
import javax.servlet.http.HttpServlet;
import javax.servlet.http.HttpServletRequest;
@@ -28,6 +29,7 @@ import org.apache.catalina.Context;
import org.apache.catalina.Wrapper;
import org.apache.catalina.startup.Tomcat;
import org.apache.catalina.startup.TomcatBaseTest;
+import org.apache.tomcat.util.buf.ByteChunk;
public class TestAsyncContextImpl extends TomcatBaseTest {
@@ -48,11 +50,90 @@ public class TestAsyncContextImpl extend
tomcat.start();
// Call the servlet once
- getUrl("http://localhost:"; + getPort() + "/");
+ ByteChunk bc = getUrl("http://localhost:"; + getPort() + "/");
+ assertEquals("OK", bc.toString());
+
+
+ assertEquals("1false2true3true4true5false", servlet.getResult());
+ }
+
+ public void testBug49567() throws Exception {
+ // Setup Tomcat instance
+ Tomcat tomcat = getTomcatInstance();
+
+ // Must have a real docBase - just use temp
+ Context ctx =
+ tomcat.addContext("/", System.getProperty("java.io.tmpdir"));
+
+ Bug49567Servlet servlet = new Bug49567Servlet();
+
+ Wrapper wrapper = Tomcat.addServlet(ctx, "servlet", servlet);
+ wrapper.setAsyncSupported(true);
+ ctx.addServletMapping("/", "servlet");
+
+ tomcat.start();
+
+ // Call the servlet once
+ ByteChunk bc = getUrl("http://localhost:"; + getPort() + "/");
+ assertEquals("OK", bc.toString());
assertEquals("1false2true3true4true5false", servlet.getResult());
}
+ public void testAsyncStartNoComplete() throws Exception {
+ // Setup Tomcat instance
+ Tomcat tomcat = getTomcatInstance();
+
+ // Minimise pauses during test
+ tomcat.getConnector().setAttribute(
+ "connectionTimeout", Integer.valueOf(3000));
+
+ // Must have a real docBase - just use temp
+ Context ctx =
+ tomcat.addContext("/", System.getProperty("java.io.tmpdir"));
+
+ AsyncStartNoCompleteServlet servlet =
+ new AsyncStartNoCompleteServlet();
+
+ Wrapper wrapper = Tomcat.addServlet(ctx, "servlet", servlet);
+ wrapper.setAsyncSupported(true);
+ ctx.addServletMapping("/", "servlet");
+
+ tomcat.start();
+
+ // Call the servlet the first time
+ ByteChunk bc1 = getUrl("http://localhost:"; + getPort() +
+ "/?echo=run1");
+ assertEquals("OK-run1", bc1.toString());
+
+ // Call the servlet the second time with a request parameter
+ ByteChunk bc2 = getUrl("http://localhost:"; + getPort() +
+ "/?echo=run2");
+ assertEquals("OK-run2", bc2.toString());
+ }
+
+ public void testAsyncStartWithComplete() throws Exception {
+ // Setup Tomcat instance
+ Tomcat tomcat = getTomcatInstance();
+
+ // Must have a real docBase - just use temp
+ Context ctx =
+ tomcat.addContext("/", System.getProperty("java.io.tmpdir"));
+
+ AsyncStartWithCompleteServlet servlet =
+ new AsyncStartWithCompleteServlet();
+
+ Wrapper wrapper = Tomcat.addServlet(ctx, "servlet", servlet);
+ wrapper.setAsyncSupported(true);
+ ctx.addServletMapping("/", "servlet");
+
+ tomcat.start();
+
+ // Call the servlet once
+ ByteChunk bc = getUrl("http://localhost:"; + getPort() + "/");
+ assertEquals("OK", bc.toString());
+ }
+
private static class Bug49528Servlet extends HttpServlet {
private static final long serialVersionUID = 1L;
@@ -83,12 +164,15 @@ public class TestAsyncContextImpl extend
Thread.sleep(1000);
result.append('4');
result.append(req.isAsyncStarted());
+ resp.setContentType("text/plain");
+ resp.getWriter().print("OK");
req.getAsyncContext().complete();
result.append('5');
result.append(req.isAsyncStarted());
} catch (InterruptedException e) {
- // TODO Auto-generated catch block
- e.printStackTrace();
+ result.append(e);
+ } catch (IOException e) {
+ result.append(e);
}
}
});
@@ -97,4 +181,96 @@ public class TestAsyncContextImpl extend
req.getMethod();
}
}
+
+ private static class Bug49567Servlet extends HttpServlet {
+
+ private static final long serialVersionUID = 1L;
+
+ private StringBuilder result = new StringBuilder();
+
+ public String getResult() {
+ return result.toString();
+ }
+
+ @Override
+ protected void doGet(final HttpServletRequest req,
+ final HttpServletResponse resp)
+ throws ServletException, IOException {
+
+ result.append('1');
+ result.append(req.isAsyncStarted());
+ req.startAsync();
+ result.append('2');
+ result.append(req.isAsyncStarted());
+
+ req.getAsyncContext().start(new Runnable() {
+ @Override
+ public void run() {
+ Thread t = new Thread(new Runnable() {
+ @Override
+ public void run() {
+ try {
+ result.append('3');
+ result.append(req.isAsyncStarted());
+ Thread.sleep(1000);
+ result.append('4');
+ result.append(req.isAsyncStarted());
+ resp.setContentType("text/plain");
+ resp.getWriter().print("OK");
+ req.getAsyncContext().complete();
+ result.append('5');
+ result.append(req.isAsyncStarted());
+ } catch (InterruptedException e) {
+ result.append(e);
+ } catch (IOException e) {
+ result.append(e);
+ }
+ }
+ });
+ t.start();
+ }
+ });
+ // Pointless method call so there is somewhere to put a break point
+ // when debugging
+ req.getMethod();
+ }
+ }
+
+ private static class AsyncStartNoCompleteServlet extends HttpServlet {
+
+ private static final long serialVersionUID = 1L;
+
+ @Override
+ protected void doGet(final HttpServletRequest req,
+ final HttpServletResponse resp)
+ throws ServletException, IOException {
+
+ String echo = req.getParameter("echo");
+ AsyncContext actxt = req.startAsync();
+ resp.setContentType("text/plain");
+ resp.getWriter().print("OK");
+ if (echo != null) {
+ resp.getWriter().print("-" + echo);
+ }
+ // Speed up the test by reducing the timeout
+ actxt.setTimeout(1000);
+ }
+ }
+
+ private static class AsyncStartWithCompleteServlet extends HttpServlet {
+
+ private static final long serialVersionUID = 1L;
+
+ @Override
+ protected void doGet(final HttpServletRequest req,
+ final HttpServletResponse resp)
+ throws ServletException, IOException {
+
+ AsyncContext actxt = req.startAsync();
+ resp.setContentType("text/plain");
+ resp.getWriter().print("OK");
+ actxt.complete();
+ }
+ }
+
}
Modified: tomcat/trunk/webapps/docs/changelog.xml
URL:
http://svn.apache.org/viewvc/tomcat/trunk/webapps/docs/changelog.xml?rev=966548&r1=966547&r2=966548&view=diff
==============================================================================
--- tomcat/trunk/webapps/docs/changelog.xml (original)
+++ tomcat/trunk/webapps/docs/changelog.xml Thu Jul 22 09:12:18 2010
@@ -122,11 +122,11 @@
rather than an empty string. (markt)
</fix>
<fix>
- <bug>49528</bug>: Ensure AsyncContext.isAsyncStarted() returns the
- correct value after AsyncContext.start(). Tomcat implements this
- using an internal dispatch that requires slightly different treatment
- from a standard dispatch to ensure the correct value is returned.
- (markt)
+ <bug>49528</bug>, <bug>49567</bug>: Ensure that
+ <code>AsyncContext.isAsyncStarted()</code> returns the correct value
+ after <code>AsyncContext.start()</code> and that if
+ <code>AsyncContext.complete()</code> is called on a separate thread
that
+ it is handled correctly. (markt)
</fix>
<fix>
<bug>49530</bug>: Contexts and Servlets not stopped when Tomcat is shut
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscr...@tomcat.apache.org
For additional commands, e-mail: dev-h...@tomcat.apache.org
