Mailing-List: contact dev-help@tomcat.apache.org; run by ezmlm
Precedence: bulk
Reply-To: "Tomcat Developers List" <dev@tomcat.apache.org>
Content-Type: text/plain; charset="utf-8"
MIME-Version: 1.0
Content-Transfer-Encoding: 7bit
Subject: svn commit: r1178449 - in /tomcat/trunk:
 java/org/apache/catalina/connector/
 java/org/apache/coyote/http11/ test/org/apache/catalina/comet/
Date: Mon, 03 Oct 2011 16:42:20 -0000
To: dev@tomcat.apache.org
From: markt@apache.org
Message-Id: <20111003164220.AC5152388A5F@eris.apache.org>

Author: markt
Date: Mon Oct  3 16:42:19 2011
New Revision: 1178449

URL: http://svn.apache.org/viewvc?rev=1178449&view=rev
Log:
Fix https://issues.apache.org/bugzilla/show_bug.cgi?id=51881
Don't mark processors handling comet requests as non-comet too early.
Before this fix, finishing a comet request was processed as non-comet meaning the comet clean-up code was not executed which was likely to break processing of the next request on the connection.

Modified:
    tomcat/trunk/java/org/apache/catalina/connector/CometEventImpl.java
    tomcat/trunk/java/org/apache/catalina/connector/Request.java
    tomcat/trunk/java/org/apache/coyote/http11/AbstractHttp11Processor.java
    tomcat/trunk/java/org/apache/coyote/http11/Http11AprProcessor.java
    tomcat/trunk/java/org/apache/coyote/http11/Http11NioProcessor.java
    tomcat/trunk/java/org/apache/coyote/http11/Http11Processor.java
    tomcat/trunk/test/org/apache/catalina/comet/TestCometProcessor.java

Modified: tomcat/trunk/java/org/apache/catalina/connector/CometEventImpl.java
URL: http://svn.apache.org/viewvc/tomcat/trunk/java/org/apache/catalina/connector/CometEventImpl.java?rev=1178449&r1=1178448&r2=1178449&view=diff
==============================================================================
--- tomcat/trunk/java/org/apache/catalina/connector/CometEventImpl.java (original)
+++ tomcat/trunk/java/org/apache/catalina/connector/CometEventImpl.java Mon Oct  3 16:42:19 2011
@@ -93,11 +93,11 @@ public class CometEventImpl implements C
         if (request == null) {
             throw new IllegalStateException(sm.getString("cometEvent.nullRequest"));
         }
-        boolean iscomet = request.isComet();
-        request.setComet(false);
         request.finishRequest();
         response.finishResponse();
-        if (iscomet) request.cometClose();
+        if (request.isComet()) {
+            request.cometClose();
+        }
     }
 
     @Override

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=1178449&r1=1178448&r2=1178449&view=diff
==============================================================================
--- tomcat/trunk/java/org/apache/catalina/connector/Request.java (original)
+++ tomcat/trunk/java/org/apache/catalina/connector/Request.java Mon Oct  3 16:42:19 2011
@@ -2514,6 +2514,7 @@ public class Request
     
     public void cometClose() {
         coyoteRequest.action(ActionCode.COMET_CLOSE,getEvent());
+        setComet(false);
     }
     
     public void setCometTimeout(long timeout) {

Modified: tomcat/trunk/java/org/apache/coyote/http11/AbstractHttp11Processor.java
URL: http://svn.apache.org/viewvc/tomcat/trunk/java/org/apache/coyote/http11/AbstractHttp11Processor.java?rev=1178449&r1=1178448&r2=1178449&view=diff
==============================================================================
--- tomcat/trunk/java/org/apache/coyote/http11/AbstractHttp11Processor.java (original)
+++ tomcat/trunk/java/org/apache/coyote/http11/AbstractHttp11Processor.java Mon Oct  3 16:42:19 2011
@@ -723,7 +723,17 @@ public abstract class AbstractHttp11Proc
     @Override
     public final void action(ActionCode actionCode, Object param) {
         
-        if (actionCode == ActionCode.COMMIT) {
+        if (actionCode == ActionCode.CLOSE) {
+            // End the processing of the current request
+
+            try {
+                getOutputBuffer().endRequest();
+            } catch (IOException e) {
+                // Set error flag
+                error = true;
+            }
+
+        } else if (actionCode == ActionCode.COMMIT) {
             // Commit current response
 
             if (response.isCommitted())

Modified: tomcat/trunk/java/org/apache/coyote/http11/Http11AprProcessor.java
URL: http://svn.apache.org/viewvc/tomcat/trunk/java/org/apache/coyote/http11/Http11AprProcessor.java?rev=1178449&r1=1178448&r2=1178449&view=diff
==============================================================================
--- tomcat/trunk/java/org/apache/coyote/http11/Http11AprProcessor.java (original)
+++ tomcat/trunk/java/org/apache/coyote/http11/Http11AprProcessor.java Mon Oct  3 16:42:19 2011
@@ -273,21 +273,7 @@ public class Http11AprProcessor extends 
 
         long socketRef = socket.getSocket().longValue();
         
-        if (actionCode == ActionCode.CLOSE) {
-            // Close
-
-            // End the processing of the current request, and stop any further
-            // transactions with the client
-
-            comet = false;
-            try {
-                outputBuffer.endRequest();
-            } catch (IOException e) {
-                // Set error flag
-                error = true;
-            }
-
-        } else if (actionCode == ActionCode.REQ_HOST_ADDR_ATTRIBUTE) {
+        if (actionCode == ActionCode.REQ_HOST_ADDR_ATTRIBUTE) {
 
             // Get remote host address
             if (remoteAddr == null && (socketRef != 0)) {

Modified: tomcat/trunk/java/org/apache/coyote/http11/Http11NioProcessor.java
URL: http://svn.apache.org/viewvc/tomcat/trunk/java/org/apache/coyote/http11/Http11NioProcessor.java?rev=1178449&r1=1178448&r2=1178449&view=diff
==============================================================================
--- tomcat/trunk/java/org/apache/coyote/http11/Http11NioProcessor.java (original)
+++ tomcat/trunk/java/org/apache/coyote/http11/Http11NioProcessor.java Mon Oct  3 16:42:19 2011
@@ -95,14 +95,7 @@ public class Http11NioProcessor extends 
      */
     protected NioEndpoint.SendfileData sendfileData = null;
 
-    /**
-     * Closed flag, a Comet async thread can 
-     * signal for this Nio processor to be closed and recycled instead
-     * of waiting for a timeout.
-     * Closed by HttpServletResponse.getWriter().close()
-     */
-    protected boolean cometClose = false;
-    
+
     /**
      * Socket associated with the current connection.
      */
@@ -289,7 +282,6 @@ public class Http11NioProcessor extends 
     @Override
     public void recycleInternal() {
         socket = null;
-        cometClose = false;
         comet = false;
         sendfileData = null;
     }
@@ -307,22 +299,7 @@ public class Http11NioProcessor extends 
     @Override
     public void actionInternal(ActionCode actionCode, Object param) {
 
-        if (actionCode == ActionCode.CLOSE) {
-            // Close
-            // End the processing of the current request, and stop any further
-            // transactions with the client
-
-            comet = false;
-            cometClose = true;
-
-            try {
-                outputBuffer.endRequest();
-            } catch (IOException e) {
-                // Set error flag
-                error = true;
-            }
-
-        } else if (actionCode == ActionCode.REQ_HOST_ADDR_ATTRIBUTE) {
+        if (actionCode == ActionCode.REQ_HOST_ADDR_ATTRIBUTE) {
 
             // Get remote host address
             if ((remoteAddr == null) && (socket != null)) {
@@ -459,10 +436,13 @@ public class Http11NioProcessor extends 
             if (socket==null || socket.getSocket().getAttachment(false)==null) return;
             NioEndpoint.KeyAttachment attach = (NioEndpoint.KeyAttachment)socket.getSocket().getAttachment(false);
             attach.setCometOps(NioEndpoint.OP_CALLBACK);
-            //notify poller if not on a tomcat thread
             RequestInfo rp = request.getRequestProcessor();
-            if ( rp.getStage() != org.apache.coyote.Constants.STAGE_SERVICE ) //async handling
+            if (rp.getStage() != org.apache.coyote.Constants.STAGE_SERVICE) {
+                // Close event for this processor triggered by request
+                // processing in another processor, a non-Tomcat thread (i.e.
+                // an application controlled thread) or similar.
                 socket.getSocket().getPoller().add(socket.getSocket());
+            }
         } else if (actionCode == ActionCode.COMET_SETTIMEOUT) {
             if (param==null) return;
             if (socket==null || socket.getSocket().getAttachment(false)==null) return;

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=1178449&r1=1178448&r2=1178449&view=diff
==============================================================================
--- tomcat/trunk/java/org/apache/coyote/http11/Http11Processor.java (original)
+++ tomcat/trunk/java/org/apache/coyote/http11/Http11Processor.java Mon Oct  3 16:42:19 2011
@@ -244,19 +244,7 @@ public class Http11Processor extends Abs
     @Override
     public void actionInternal(ActionCode actionCode, Object param) {
 
-        if (actionCode == ActionCode.CLOSE) {
-            // Close
-            // End the processing of the current request, and stop any further
-            // transactions with the client
-
-            try {
-                outputBuffer.endRequest();
-            } catch (IOException e) {
-                // Set error flag
-                error = true;
-            }
-
-        } else if (actionCode == ActionCode.REQ_SSL_ATTRIBUTE ) {
+        if (actionCode == ActionCode.REQ_SSL_ATTRIBUTE ) {
 
             try {
                 if (sslSupport != null) {

Modified: tomcat/trunk/test/org/apache/catalina/comet/TestCometProcessor.java
URL: http://svn.apache.org/viewvc/tomcat/trunk/test/org/apache/catalina/comet/TestCometProcessor.java?rev=1178449&r1=1178448&r2=1178449&view=diff
==============================================================================
--- tomcat/trunk/test/org/apache/catalina/comet/TestCometProcessor.java (original)
+++ tomcat/trunk/test/org/apache/catalina/comet/TestCometProcessor.java Mon Oct  3 16:42:19 2011
@@ -31,17 +31,90 @@ import javax.servlet.http.HttpSession;
 import static org.junit.Assert.assertEquals;
 import static org.junit.Assert.assertNotNull;
 import static org.junit.Assert.assertTrue;
+import static org.junit.Assert.fail;
 
 import org.junit.Test;
 
 import org.apache.catalina.Context;
 import org.apache.catalina.comet.CometEvent.EventType;
+import org.apache.catalina.connector.CometEventImpl;
+import org.apache.catalina.connector.Request;
+import org.apache.catalina.connector.Response;
 import org.apache.catalina.startup.Tomcat;
 import org.apache.catalina.startup.TomcatBaseTest;
+import org.apache.catalina.valves.ValveBase;
 
 public class TestCometProcessor extends TomcatBaseTest {
 
     @Test
+    public void testAsyncClose() throws Exception {
+        
+        if (!isCometSupported()) {
+            return;
+        }
+
+        // Setup Tomcat instance
+        Tomcat tomcat = getTomcatInstance();
+        Context root = tomcat.addContext("", TEMP_DIR);
+        Tomcat.addServlet(root, "comet", new SimpleCometServlet());
+        root.addServletMapping("/comet", "comet");
+        Tomcat.addServlet(root, "hello", new HelloWorldServlet());
+        root.addServletMapping("/hello", "hello");
+        root.getPipeline().addValve(new AsyncCometCloseValve());
+        tomcat.getConnector().setProperty("connectionTimeout", "5000");
+        tomcat.start();
+        
+        // Create connection to Comet servlet
+        final Socket socket =
+            SocketFactory.getDefault().createSocket("localhost", getPort());
+        socket.setSoTimeout(5000);
+        
+        final OutputStream os = socket.getOutputStream();
+        String requestLine = "POST http://localhost:" + getPort() +
+                "/comet HTTP/1.1\r\n";
+        os.write(requestLine.getBytes());
+        os.write("transfer-encoding: chunked\r\n".getBytes());
+        os.write("\r\n".getBytes());
+
+        InputStream is = socket.getInputStream();
+        ResponseReaderThread readThread = new ResponseReaderThread(is);
+        readThread.start();
+        
+        // Wait for the comet request/response to finish
+        int count = 0;
+        while (count < 10 && !readThread.getResponse().endsWith("0\r\n\r\n")) {
+            Thread.sleep(500);
+            count++;
+        }
+        
+        if (count == 10) {
+            fail("Comet request did not complete");
+        }
+        
+        // Send a standard HTTP request on the same connection
+        requestLine = "GET http://localhost:" + getPort() +
+                "/hello HTTP/1.1\r\n";
+        os.write(requestLine.getBytes());
+        os.write("\r\n".getBytes());
+        
+        // Check for the expected response
+        count = 0;
+        while (count < 10 && !readThread.getResponse().contains(
+                HelloWorldServlet.RESPONSE_TEXT)) {
+            Thread.sleep(500);
+            count++;
+        }
+
+        if (count == 10) {
+            fail("Non-comet request did not complete");
+        }
+
+        readThread.join();
+        os.close();
+        is.close();
+    }
+
+    @Test
     public void testSimpleCometClient() throws Exception {
         
         if (!isCometSupported()) {
@@ -286,4 +359,41 @@ public class TestCometProcessor extends 
             }
         }
     }
+
+    private static class AsyncCometCloseValve extends ValveBase {
+
+        @Override
+        public void invoke(Request request, Response response)
+                throws IOException, ServletException {
+            
+            CometEventImpl event = new CometEventImpl(request, response);
+            
+            getNext().invoke(request, response);
+            
+            if (request.isComet()) {
+                Thread t = new AsyncCometCloseThread(event);
+                t.start();
+            }
+        }
+    }
+    
+    private static class AsyncCometCloseThread extends Thread {
+
+        private CometEvent event;
+        
+        public AsyncCometCloseThread(CometEvent event) {
+            this.event = event;
+        }
+        
+        @Override
+        public void run() {
+            try {
+                Thread.sleep(2000);
+                event.close();
+            } catch (Exception e) {
+                // Test should fail. Report what went wrong.
+                e.printStackTrace();
+            }
+        }
+    }
 }


---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@tomcat.apache.org
For additional commands, e-mail: dev-help@tomcat.apache.org