tomcat-dev mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From ma...@apache.org
Subject svn commit: r966548 - in /tomcat/trunk: java/org/apache/catalina/connector/ java/org/apache/catalina/core/ java/org/apache/coyote/http11/ test/org/apache/catalina/core/ webapps/docs/
Date Thu, 22 Jul 2010 09:12:19 GMT
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-unsubscribe@tomcat.apache.org
For additional commands, e-mail: dev-help@tomcat.apache.org


Mime
View raw message