Fixes #12104 - Enable ee9 ErrorHandlerTest again

+ Fix empty `Connection:` response header issue seen on HTTP/1.0
jetty · Jul 29, 2024 · 88b145d · 88b145d
1 parent e755458
commit 88b145d
Show file tree

Hide file tree

Showing 2 changed files with 49 additions and 40 deletions.
diff --git a/jetty-core/jetty-server/src/main/java/org/eclipse/jetty/server/internal/HttpConnection.java b/jetty-core/jetty-server/src/main/java/org/eclipse/jetty/server/internal/HttpConnection.java
@@ -1384,8 +1384,18 @@ public void demand()
         @Override
         public void prepareResponse(HttpFields.Mutable headers)
         {
-            if (_connectionKeepAlive && _version == HttpVersion.HTTP_1_0 && !headers.contains(HttpHeader.CONNECTION, HttpHeaderValue.CLOSE.asString()))
-                headers.add(HttpFields.CONNECTION_KEEPALIVE);
+            // If the HTTP/1 generator is set to no longer be persistent then the
+            // logic for handling HTTP/1.0 shouldn't occur.  This typically happens
+            // in the case of errors with the request or response.
+//            if (_version == HttpVersion.HTTP_1_0 && _generator.isPersistent())
+            {
+                // attempt to cover HTTP/1.0 case where the Connection response header
+                // doesn't have a close value (set by the application) and it needs to
+                // be represented as a `Connection: keep-alive` response instead due
+                // to the existence of the `Connection: keep-alive` request header
+                if (_connectionKeepAlive && !headers.contains(HttpHeader.CONNECTION, HttpHeaderValue.CLOSE.asString()))
+                    headers.add(HttpFields.CONNECTION_KEEPALIVE);
+            }
         }
 
         @Override

diff --git a/jetty-ee9/jetty-ee9-nested/src/test/java/org/eclipse/jetty/ee9/nested/ErrorHandlerTest.java b/jetty-ee9/jetty-ee9-nested/src/test/java/org/eclipse/jetty/ee9/nested/ErrorHandlerTest.java
@@ -31,14 +31,14 @@
 import org.eclipse.jetty.http.BadMessageException;
 import org.eclipse.jetty.http.HttpHeader;
 import org.eclipse.jetty.http.HttpTester;
+import org.eclipse.jetty.io.Content;
 import org.eclipse.jetty.logging.StacklessLogging;
 import org.eclipse.jetty.server.LocalConnector;
 import org.eclipse.jetty.server.Server;
 import org.eclipse.jetty.util.BufferUtil;
 import org.eclipse.jetty.util.ajax.JSON;
 import org.junit.jupiter.api.AfterEach;
 import org.junit.jupiter.api.BeforeEach;
-import org.junit.jupiter.api.Disabled;
 import org.junit.jupiter.api.Test;
 import org.junit.jupiter.params.ParameterizedTest;
 import org.junit.jupiter.params.provider.ValueSource;
@@ -55,7 +55,6 @@
 import static org.hamcrest.Matchers.nullValue;
 import static org.junit.jupiter.api.Assertions.assertTrue;
 
-@Disabled // TODO
 public class ErrorHandlerTest
 {
     StacklessLogging stacklessLogging;
@@ -281,17 +280,21 @@ public void test404PostHttp11() throws Exception
     @Test
     public void test404PostCantConsumeHttp10() throws Exception
     {
-        String rawResponse = connector.getResponse(
-            "POST / HTTP/1.0\r\n" +
-                "Host: Localhost\r\n" +
-                "Accept: text/html\r\n" +
-                "Content-Length: 100\r\n" +
-                "Connection: keep-alive\r\n" +
-                "\r\n" +
-                "0123456789");
-
+        String rawRequest = """
+            POST / HTTP/1.0\r
+            Host: Localhost\r
+            Accept: text/html\r
+            Content-Length: 100\r
+            Connection: keep-alive\r
+            \r
+            0123456789
+            """;
+
+        String rawResponse = connector.getResponse(rawRequest);
         HttpTester.Response response = HttpTester.parseResponse(rawResponse);
 
+        dump(response);
+
         assertThat(response.getStatus(), is(404));
         assertThat(response.getField(HttpHeader.CONTENT_LENGTH).getIntValue(), greaterThan(0));
         assertThat(response.get(HttpHeader.CONTENT_TYPE), containsString("text/html;charset=ISO-8859-1"));
@@ -308,7 +311,6 @@ public void test404PostCantConsumeHttp11() throws Exception
                 "Host: Localhost\r\n" +
                 "Accept: text/html\r\n" +
                 "Content-Length: 100\r\n" +
-                "Connection: keep-alive\r\n" + // This is not need by HTTP/1.1 but sometimes sent anyway
                 "\r\n" +
                 "0123456789");
 
@@ -703,7 +705,7 @@ public void testErrorContextRecycle() throws Exception
         context.setErrorHandler(new ErrorHandler()
         {
             @Override
-            public void handle(String target, Request baseRequest, HttpServletRequest request, HttpServletResponse response) throws IOException, ServletException
+            public void handle(String target, Request baseRequest, HttpServletRequest request, HttpServletResponse response) throws IOException
             {
                 baseRequest.setHandled(true);
                 response.getOutputStream().println("Context Error");
@@ -717,35 +719,32 @@ public void handle(String target, Request baseRequest, HttpServletRequest reques
                 response.sendError(444);
             }
         });
-
-        context.setErrorHandler(new ErrorHandler()
+        server.setErrorHandler((request, response, callback) ->
         {
-            @Override
-            public void handle(String target, Request baseRequest, HttpServletRequest request, HttpServletResponse response) throws IOException, ServletException
-            {
-                baseRequest.setHandled(true);
-                response.getOutputStream().println("Server Error");
-            }
+            Content.Sink.write(response, true, "Server Error", callback);
+            return true;
         });
 
         server.start();
 
-        LocalConnector.LocalEndPoint connection = connector.connect();
-        connection.addInputAndExecute(BufferUtil.toBuffer(
-            "GET /foo/test HTTP/1.1\r\n" +
-                "Host: Localhost\r\n" +
-                "\r\n"));
-        String response = connection.getResponse();
-
-        assertThat(response, containsString("HTTP/1.1 444 444"));
-        assertThat(response, containsString("Context Error"));
-
-        connection.addInputAndExecute(BufferUtil.toBuffer(
-            "GET /test HTTP/1.1\r\n" +
-                "Host: Localhost\r\n" +
-                "\r\n"));
-        response = connection.getResponse();
-        assertThat(response, containsString("HTTP/1.1 404 Not Found"));
-        assertThat(response, containsString("Server Error"));
+        try (LocalConnector.LocalEndPoint connection = connector.connect())
+        {
+            connection.addInputAndExecute(BufferUtil.toBuffer(
+                "GET /foo/test HTTP/1.1\r\n" +
+                    "Host: Localhost\r\n" +
+                    "\r\n"));
+            String response = connection.getResponse();
+
+            assertThat(response, containsString("HTTP/1.1 444 444"));
+            assertThat(response, containsString("Context Error"));
+
+            connection.addInputAndExecute(BufferUtil.toBuffer(
+                "GET /test HTTP/1.1\r\n" +
+                    "Host: Localhost\r\n" +
+                    "\r\n"));
+            response = connection.getResponse();
+            assertThat(response, containsString("HTTP/1.1 404 Not Found"));
+            assertThat(response, containsString("Server Error"));
+        }
     }
 }