rework and cleanup code for ensureConsumeAllOrNotPersistent

Signed-off-by: Lachlan Roberts <lachlan@webtide.com>

jetty-core/jetty-server/src/main/java/org/eclipse/jetty/server/Response.java

@@ -19,6 +19,8 @@ import java.nio.ByteBuffer;
 import java.nio.channels.WritableByteChannel;
 import java.nio.charset.StandardCharsets;
 import java.util.ListIterator;
+import java.util.stream.Collectors;
+import java.util.stream.Stream;
 
 import org.eclipse.jetty.http.BadMessageException;
 import org.eclipse.jetty.http.CookieCompliance;
@@ -26,6 +28,7 @@ import org.eclipse.jetty.http.HttpCookie;
 import org.eclipse.jetty.http.HttpField;
 import org.eclipse.jetty.http.HttpFields;
 import org.eclipse.jetty.http.HttpHeader;
+import org.eclipse.jetty.http.HttpHeaderValue;
 import org.eclipse.jetty.http.HttpStatus;
 import org.eclipse.jetty.io.QuietException;
 import org.eclipse.jetty.server.handler.ErrorProcessor;
@@ -270,6 +273,8 @@ public interface Response extends Content.Writer
             return;
         }
 
+        Response.ensureConsumeAllOrNotPersistent(request, response);
+
         if (status <= 0)
             status = HttpStatus.INTERNAL_SERVER_ERROR_500;
         if (message == null)
@@ -317,6 +322,79 @@ public interface Response extends Content.Writer
         return -1;
     }
 
+    static void ensureConsumeAllOrNotPersistent(Request request, Response response)
+    {
+        switch (request.getConnectionMetaData().getHttpVersion())
+        {
+            case HTTP_1_0:
+                if (consumeAll(request))
+                    return;
+
+                // Remove any keep-alive value in Connection headers
+                response.getHeaders().computeField(HttpHeader.CONNECTION, (h, fields) ->
+                {
+                    if (fields == null || fields.isEmpty())
+                        return null;
+                    String v = fields.stream()
+                        .flatMap(field -> Stream.of(field.getValues()).filter(s -> !HttpHeaderValue.KEEP_ALIVE.is(s)))
+                        .collect(Collectors.joining(", "));
+                    if (StringUtil.isEmpty(v))
+                        return null;
+
+                    return new HttpField(HttpHeader.CONNECTION, v);
+                });
+                break;
+
+            case HTTP_1_1:
+                if (consumeAll(request))
+                    return;
+
+                // Add close value to Connection headers
+                response.getHeaders().computeField(HttpHeader.CONNECTION, (h, fields) ->
+                {
+                    if (fields == null || fields.isEmpty())
+                        return HttpFields.CONNECTION_CLOSE;
+
+                    if (fields.stream().anyMatch(f -> f.contains(HttpHeaderValue.CLOSE.asString())))
+                    {
+                        if (fields.size() == 1)
+                        {
+                            HttpField f = fields.get(0);
+                            if (HttpFields.CONNECTION_CLOSE.equals(f))
+                                return f;
+                        }
+
+                        return new HttpField(HttpHeader.CONNECTION, fields.stream()
+                            .flatMap(field -> Stream.of(field.getValues()).filter(s -> !HttpHeaderValue.KEEP_ALIVE.is(s)))
+                            .collect(Collectors.joining(", ")));
+                    }
+
+                    return new HttpField(HttpHeader.CONNECTION,
+                        Stream.concat(fields.stream()
+                                    .flatMap(field -> Stream.of(field.getValues()).filter(s -> !HttpHeaderValue.KEEP_ALIVE.is(s))),
+                                Stream.of(HttpHeaderValue.CLOSE.asString()))
+                            .collect(Collectors.joining(", ")));
+                });
+                break;
+
+            default:
+                break;
+        }
+    }
+
+    static boolean consumeAll(Request request)
+    {
+        while (true)
+        {
+            Content content = request.readContent();
+            if (content == null)
+                return false;
+            content.release();
+            if (content.isLast())
+                return true;
+        }
+    }
+
     class Wrapper implements Response
     {
         private final Request _request;

jetty-ee10/jetty-ee10-servlet/src/main/java/org/eclipse/jetty/ee10/servlet/ServletChannel.java

@@ -36,6 +36,7 @@ import org.eclipse.jetty.server.Connector;
 import org.eclipse.jetty.server.Content;
 import org.eclipse.jetty.server.HttpConfiguration;
 import org.eclipse.jetty.server.Request;
+import org.eclipse.jetty.server.Response;
 import org.eclipse.jetty.server.Server;
 import org.eclipse.jetty.server.handler.ContextHandler;
 import org.eclipse.jetty.util.Blocking;
@@ -468,7 +469,7 @@ public class ServletChannel implements Runnable
                             // from the failed dispatch, then we try to consume it here and if we fail we add a
                             // Connection:close.  This can't be deferred to COMPLETE as the response will be committed
                             // by then.
-                            ensureConsumeAllOrNotPersistent();
+                            Response.ensureConsumeAllOrNotPersistent(_request, _request.getResponse());
 
                             ContextHandler.Context context = (ContextHandler.Context)_request.getAttribute(ErrorHandler.ERROR_CONTEXT);
                             Request.Processor errorProcessor = ErrorHandler.getErrorProcessor(getServer(), context == null ? null : context.getContextHandler());
@@ -554,7 +555,7 @@ public class ServletChannel implements Runnable
 
                             // Indicate Connection:close if we can't consume all.
                             if (getResponse().getStatus() >= 200)
-                                ensureConsumeAllOrNotPersistent();
+                                Response.ensureConsumeAllOrNotPersistent(_request, _request.getResponse());
                         }
 
 
@@ -599,19 +600,6 @@ public class ServletChannel implements Runnable
         return !suspended;
     }
 
-    private void ensureConsumeAllOrNotPersistent()
-    {
-        while (true)
-        {
-            Content content = getRequest().readContent();
-            if (content == null)
-                break;
-            content.release();
-            if (content.isLast())
-                break;
-        }
-    }
-
     /**
      * @param message the error message.
      * @return true if we have sent an error, false if we have aborted.

jetty-ee10/jetty-ee10-servlet/src/test/java/org/eclipse/jetty/ee10/servlet/ErrorPageTest.java

@@ -49,7 +49,6 @@ import org.eclipse.jetty.server.Server;
 import org.hamcrest.Matchers;
 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.slf4j.Logger;
 import org.slf4j.LoggerFactory;
@@ -420,19 +419,19 @@ public class ErrorPageTest
         }
     }
 
-    @Disabled
     @Test
     public void testNoop() throws Exception
     {
+        // The ServletContextHandler does not handle so should go to the servers ErrorProcessor.
         String response = _connector.getResponse("GET /noop/info HTTP/1.0\r\n\r\n");
         assertThat(response, Matchers.containsString("HTTP/1.1 404 Not Found"));
-        assertThat(response, Matchers.containsString("DISPATCH: ERROR"));
+        assertThat(response, not(Matchers.containsString("DISPATCH: ERROR")));
-        assertThat(response, Matchers.containsString("ERROR_PAGE: /GlobalErrorPage"));
+        assertThat(response, not(Matchers.containsString("ERROR_PAGE: /GlobalErrorPage")));
-        assertThat(response, Matchers.containsString("ERROR_CODE: 404"));
+        assertThat(response, not(Matchers.containsString("ERROR_CODE: 404")));
-        assertThat(response, Matchers.containsString("ERROR_EXCEPTION: null"));
+        assertThat(response, not(Matchers.containsString("ERROR_EXCEPTION: null")));
-        assertThat(response, Matchers.containsString("ERROR_EXCEPTION_TYPE: null"));
+        assertThat(response, not(Matchers.containsString("ERROR_EXCEPTION_TYPE: null")));
-        assertThat(response, Matchers.containsString("ERROR_SERVLET: org.eclipse.jetty.ee10.servlet.DefaultServlet-"));
+        assertThat(response, not(Matchers.containsString("ERROR_SERVLET: org.eclipse.jetty.ee10.servlet.DefaultServlet-")));
-        assertThat(response, Matchers.containsString("ERROR_REQUEST_URI: /noop/info"));
+        assertThat(response, not(Matchers.containsString("ERROR_REQUEST_URI: /noop/info")));
     }
 
     @Test