Fix https://issues.apache.org/bugzilla/show_bug.cgi?id=52009

Fix the NPE if an error occurs during comet processing
Add test cases for errors during comet processing
Ensure access log entries are made if an error occurs

git-svn-id: https://svn.apache.org/repos/asf/tomcat/trunk@1183494 13f79535-47bb-0310-9956-ffa450edef68

diff --git a/java/org/apache/catalina/connector/CoyoteAdapter.java b/java/org/apache/catalina/connector/CoyoteAdapter.java
index 379935a..e38fcdc 100644 (file)
--- a/java/org/apache/catalina/connector/CoyoteAdapter.java
+++ b/java/org/apache/catalina/connector/CoyoteAdapter.java
@@ -249,6 +249,10 @@ public class CoyoteAdapter implements Adapter {
             req.getRequestProcessor().setWorkerThreadName(null);
             // Recycle the wrapper request and response
             if (error || response.isClosed() || !request.isComet()) {
+                ((Context) request.getMappingData().context).logAccess(
+                        request, response,
+                        System.currentTimeMillis() - req.getStartTime(),
+                        false);
                 request.recycle();
                 request.setFilterChain(null);
                 response.recycle();
@@ -430,9 +434,12 @@ public class CoyoteAdapter implements Adapter {
             } else if (!comet) {
                 request.finishRequest();
                 response.finishResponse();
-                if (postParseSuccess) {
+                if (postParseSuccess &&
+                        request.getMappingData().context != null) {
                     // Log only if processing was invoked.
                     // If postParseRequest() failed, it has already logged it.
+                    // If context is null this was the start of a comet request
+                    // that failed and has already been logged.
                     ((Context) request.getMappingData().context).logAccess(
                             request, response,
                             System.currentTimeMillis() - req.getStartTime(),

diff --git a/test/org/apache/catalina/comet/TestCometProcessor.java b/test/org/apache/catalina/comet/TestCometProcessor.java
index 67cb343..481bdd3 100644 (file)
--- a/test/org/apache/catalina/comet/TestCometProcessor.java
+++ b/test/org/apache/catalina/comet/TestCometProcessor.java
@@ -36,12 +36,14 @@ import static org.junit.Assert.fail;
 import org.junit.Test;
 
 import org.apache.catalina.Context;
+import org.apache.catalina.Wrapper;
 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.TesterAccessLogValve;
 import org.apache.catalina.valves.ValveBase;
 
 public class TestCometProcessor extends TomcatBaseTest {
@@ -116,7 +118,25 @@ public class TestCometProcessor extends TomcatBaseTest {
 
     @Test
     public void testSimpleCometClient() throws Exception {
+        doSimpleCometTest(null);
+    }
+
+    @Test
+    public void testSimpleCometClientBeginFail() throws Exception {
+        doSimpleCometTest(SimpleCometServlet.FAIL_ON_BEGIN);
+    }
 
+    @Test
+    public void testSimpleCometClientReadFail() throws Exception {
+        doSimpleCometTest(SimpleCometServlet.FAIL_ON_READ);
+    }
+
+    @Test
+    public void testSimpleCometClientEndFail() throws Exception {
+        doSimpleCometTest(SimpleCometServlet.FAIL_ON_END);
+    }
+
+    private void doSimpleCometTest(String initParam) throws Exception {
         if (!isCometSupported()) {
             return;
         }
@@ -124,8 +144,15 @@ public class TestCometProcessor extends TomcatBaseTest {
         // Setup Tomcat instance
         Tomcat tomcat = getTomcatInstance();
         Context root = tomcat.addContext("", TEMP_DIR);
-        Tomcat.addServlet(root, "comet", new SimpleCometServlet());
+        Wrapper w = Tomcat.addServlet(root, "comet", new SimpleCometServlet());
+        if (initParam != null) {
+            w.addInitParameter(initParam, "true");
+        }
         root.addServletMapping("/", "comet");
+
+        TesterAccessLogValve alv = new TesterAccessLogValve();
+        root.getPipeline().addValve(alv);
+
         tomcat.start();
 
         // Create connection to Comet servlet
@@ -151,36 +178,51 @@ public class TestCometProcessor extends TomcatBaseTest {
         os.close();
         is.close();
 
-        // Validate response
         String[] response = readThread.getResponse().split("\r\n");
-        assertEquals("HTTP/1.1 200 OK", response[0]);
-        assertEquals("Server: Apache-Coyote/1.1", response[1]);
-        assertTrue(response[2].startsWith("Set-Cookie: JSESSIONID="));
-        assertEquals("Content-Type: text/plain;charset=ISO-8859-1", response[3]);
-        assertEquals("Transfer-Encoding: chunked", response[4]);
-        assertTrue(response[5].startsWith("Date: "));
-        assertEquals("", response[6]);
-        assertEquals("7", response[7]);
-        assertEquals("BEGIN", response[8]);
-        assertEquals("", response[9]);
-        assertEquals("17", response[10]);
-        assertEquals("Client: READ: 4 bytes", response[11]);
-        assertEquals("", response[12]);
-        assertEquals("17", response[13]);
-        assertEquals("Client: READ: 4 bytes", response[14]);
-        assertEquals("", response[15]);
-        assertEquals("17", response[16]);
-        assertEquals("Client: READ: 4 bytes", response[17]);
-        assertEquals("", response[18]);
-        assertEquals("17", response[19]);
-        assertEquals("Client: READ: 4 bytes", response[20]);
-        assertEquals("", response[21]);
-        assertEquals("d", response[22]);
-        assertEquals("Client: END", response[23]);
-        assertEquals("", response[24]);
-        assertEquals("0", response[25]);
-        // Expect 26 lines
-        assertEquals(26, response.length);
+        if (initParam == null) {
+            // Normal response expected
+            // Validate response
+            assertEquals("HTTP/1.1 200 OK", response[0]);
+            assertEquals("Server: Apache-Coyote/1.1", response[1]);
+            assertTrue(response[2].startsWith("Set-Cookie: JSESSIONID="));
+            assertEquals("Content-Type: text/plain;charset=ISO-8859-1", response[3]);
+            assertEquals("Transfer-Encoding: chunked", response[4]);
+            assertTrue(response[5].startsWith("Date: "));
+            assertEquals("", response[6]);
+            assertEquals("7", response[7]);
+            assertEquals("BEGIN", response[8]);
+            assertEquals("", response[9]);
+            assertEquals("17", response[10]);
+            assertEquals("Client: READ: 4 bytes", response[11]);
+            assertEquals("", response[12]);
+            assertEquals("17", response[13]);
+            assertEquals("Client: READ: 4 bytes", response[14]);
+            assertEquals("", response[15]);
+            assertEquals("17", response[16]);
+            assertEquals("Client: READ: 4 bytes", response[17]);
+            assertEquals("", response[18]);
+            assertEquals("17", response[19]);
+            assertEquals("Client: READ: 4 bytes", response[20]);
+            assertEquals("", response[21]);
+            assertEquals("d", response[22]);
+            assertEquals("Client: END", response[23]);
+            assertEquals("", response[24]);
+            assertEquals("0", response[25]);
+            // Expect 26 lines
+            assertEquals(26, response.length);
+        } else {
+            // Failure expected only expected for the fail on begin
+            // Failure at any later stage and the reponse headers (including the
+            // 200 response code will already have been sent to the client
+            if (initParam == SimpleCometServlet.FAIL_ON_BEGIN) {
+                assertEquals("HTTP/1.1 500 Internal Server Error", response[0]);
+                alv.validateAccessLog(1, 500, 0, 1000);
+            } else {
+                assertEquals("HTTP/1.1 200 OK", response[0]);
+                alv.validateAccessLog(1, 200, 0, 5000);
+            }
+
+        }
     }
 
     /**
@@ -267,6 +309,26 @@ public class TestCometProcessor extends TomcatBaseTest {
 
         private static final long serialVersionUID = 1L;
 
+        public static final String FAIL_ON_BEGIN = "failOnBegin";
+        public static final String FAIL_ON_READ = "failOnRead";
+        public static final String FAIL_ON_END = "failOnEnd";
+
+        private boolean failOnBegin = false;
+        private boolean failOnRead = false;
+        private boolean failOnEnd = false;
+
+
+        @Override
+        public void init() throws ServletException {
+            failOnBegin = Boolean.valueOf(getServletConfig().getInitParameter(
+                    FAIL_ON_BEGIN)).booleanValue();
+            failOnRead = Boolean.valueOf(getServletConfig().getInitParameter(
+                    FAIL_ON_READ)).booleanValue();
+            failOnEnd = Boolean.valueOf(getServletConfig().getInitParameter(
+                    FAIL_ON_END)).booleanValue();
+        }
+
+
         @Override
         public void event(CometEvent event) throws IOException,
                 ServletException {
@@ -278,9 +340,15 @@ public class TestCometProcessor extends TomcatBaseTest {
             session.setMaxInactiveInterval(30);
 
             if (event.getEventType() == EventType.BEGIN) {
+                if (failOnBegin) {
+                    throw new IOException("Fail on begin");
+                }
                 response.setContentType("text/plain");
                 response.getWriter().print("BEGIN" + "\r\n");
             } else if (event.getEventType() == EventType.READ) {
+                if (failOnRead) {
+                    throw new IOException("Fail on read");
+                }
                 InputStream is = request.getInputStream();
                 int count = 0;
                 while (is.available() > 0) {
@@ -290,6 +358,9 @@ public class TestCometProcessor extends TomcatBaseTest {
                 String msg = "READ: " + count + " bytes";
                 response.getWriter().print("Client: " + msg + "\r\n");
             } else if (event.getEventType() == EventType.END) {
+                if (failOnEnd) {
+                    throw new IOException("Fail on end");
+                }
                 String msg = "END";
                 response.getWriter().print("Client: " + msg + "\r\n");
                 event.close();