Issue #5605 unconsumed input on sendError

jetty-server/src/main/java/org/eclipse/jetty/server/HttpChannel.java

@@ -31,14 +31,18 @@
 import java.util.function.Consumer;
 import java.util.function.Function;
 import java.util.function.Supplier;
+import java.util.stream.Collectors;
+import java.util.stream.Stream;
 import javax.servlet.DispatcherType;
 import javax.servlet.RequestDispatcher;
 import javax.servlet.ServletException;
 
 import org.eclipse.jetty.http.BadMessageException;
+import org.eclipse.jetty.http.HttpField;
 import org.eclipse.jetty.http.HttpFields;
 import org.eclipse.jetty.http.HttpGenerator;
 import org.eclipse.jetty.http.HttpHeader;
+import org.eclipse.jetty.http.HttpHeaderValue;
 import org.eclipse.jetty.http.HttpScheme;
 import org.eclipse.jetty.http.HttpStatus;
 import org.eclipse.jetty.http.HttpVersion;
@@ -406,7 +410,13 @@ public boolean handle()
                             // the following is needed as you cannot trust the response code and reason
                             // as those could have been modified after calling sendError
                             Integer code = (Integer)_request.getAttribute(RequestDispatcher.ERROR_STATUS_CODE);
-                            _response.setStatus(code != null ? code : HttpStatus.INTERNAL_SERVER_ERROR_500);
+                            if (code == null)
+                                code = HttpStatus.INTERNAL_SERVER_ERROR_500;
+                            _response.setStatus(code);
+
+                            // Add Connection:close if we can't consume the input
+                            if (!_request.getHttpInput().consumeAll())
+                                ensureConnectionClose();
 
                             ContextHandler.Context context = (ContextHandler.Context)_request.getAttribute(ErrorHandler.ERROR_CONTEXT);
                             ErrorHandler errorHandler = ErrorHandler.getErrorHandler(getServer(), context == null ? null : context.getContextHandler());
@@ -492,10 +502,18 @@ public boolean handle()
 
                     case COMPLETE:
                     {
-                        if (!_response.isCommitted() && !_request.isHandled() && !_response.getHttpOutput().isClosed())
+                        if (!_response.isCommitted())
                         {
-                            _response.sendError(HttpStatus.NOT_FOUND_404);
-                            break;
+                            if (!_request.isHandled() && !_response.getHttpOutput().isClosed())
+                            {
+                                // The request was not actually handled
+                                _response.sendError(HttpStatus.NOT_FOUND_404);
+                                break;
+                            }
+                            // If content has not been consumed and we can't consume it now without blocking
+                            // then ensure we signal that the connection will be closed.
+                            if (!_request.getHttpInput().consumeAll())
+                                ensureConnectionClose();
                         }
 
                         // RFC 7230, section 3.3.
@@ -511,12 +529,7 @@ public boolean handle()
                                 break;
                             }
                         }
-
-                        // TODO Currently a blocking/aborting consumeAll is done in the handling of the TERMINATED
-                        // TODO Action triggered by the completed callback below.  It would be possible to modify the
-                        // TODO callback to do a non-blocking consumeAll at this point and only call completed when
-                        // TODO that is done.
-
+
                         // Set a close callback on the HttpOutput to make it an async callback
                         _response.completeOutput(Callback.from(() -> _state.completed(null), _state::completed));
 
@@ -545,6 +558,29 @@ public boolean handle()
         return !suspended;
     }
 
+    private void ensureConnectionClose()
+    {
+        _response.getHttpFields().computeField(HttpHeader.CONNECTION, (h, fields) ->
+        {
+            if (fields == null || fields.isEmpty())
+                return HttpConnection.CONNECTION_CLOSE;
+
+            if (fields.stream().anyMatch(f -> f.contains(HttpHeaderValue.CLOSE.asString())))
+            {
+                if (fields.size() == 1)
+                    return fields.get(0);
+
+                return new HttpField(HttpHeader.CONNECTION, fields.stream()
+                    .flatMap(field -> Stream.of(field.getValues()))
+                    .collect(Collectors.joining(", ")));
+            }
+
+            return new HttpField(HttpHeader.CONNECTION, fields.stream()
+                .flatMap(field -> Stream.of(field.getValues()))
+                .collect(Collectors.joining(", ")) + ", " + HttpHeaderValue.CLOSE.asString());
+        });
+    }
+
     private void dispatch(DispatcherType type, Dispatchable dispatchable) throws IOException, ServletException
     {
         try

jetty-server/src/main/java/org/eclipse/jetty/server/HttpChannelState.java

@@ -904,8 +904,6 @@ public void sendError(int code, String message)
                 default:
                     throw new IllegalStateException(getStatusStringLocked());
             }
-            if (_outputState != OutputState.OPEN)
-                throw new IllegalStateException("Response is " + _outputState);
 
             response.setStatus(code);
             response.errorClose();

jetty-server/src/test/java/org/eclipse/jetty/server/AsyncRequestReadTest.java

@@ -328,6 +328,7 @@ public void testPartialReadThenClose() throws Exception
 
             BufferedReader in = new BufferedReader(new InputStreamReader(socket.getInputStream()));
             assertThat(in.readLine(), containsString("HTTP/1.1 200 OK"));
+            assertThat(in.readLine(), containsString("Connection: close"));
             assertThat(in.readLine(), containsString("Content-Length:"));
             assertThat(in.readLine(), containsString("Server:"));
             in.readLine();

jetty-server/src/test/java/org/eclipse/jetty/server/ErrorHandlerTest.java

@@ -233,6 +233,46 @@ public void test404HtmlAccept() throws Exception
         assertContent(response);
     }
 
+    @Test
+    public void test404Post() throws Exception
+    {
+        String rawResponse = connector.getResponse(
+            "POST / HTTP/1.1\r\n" +
+                "Host: Localhost\r\n" +
+                "Accept: text/html\r\n" +
+                "Content-Length: 10\r\n" +
+                "\r\n" +
+                "0123456789");
+
+        HttpTester.Response response = HttpTester.parseResponse(rawResponse);
+
+        assertThat("Response status code", response.getStatus(), is(404));
+        assertThat("Response Content-Length", response.getField(HttpHeader.CONTENT_LENGTH).getIntValue(), greaterThan(0));
+        assertThat("Response Content-Type", response.get(HttpHeader.CONTENT_TYPE), containsString("text/html;charset=ISO-8859-1"));
+        assertThat(response.getField(HttpHeader.CONNECTION), nullValue());
+        assertContent(response);
+    }
+
+    @Test
+    public void test404PostCantConsume() throws Exception
+    {
+        String rawResponse = connector.getResponse(
+            "POST / HTTP/1.1\r\n" +
+                "Host: Localhost\r\n" +
+                "Accept: text/html\r\n" +
+                "Content-Length: 100\r\n" +
+                "\r\n" +
+                "0123456789");
+
+        HttpTester.Response response = HttpTester.parseResponse(rawResponse);
+
+        assertThat("Response status code", response.getStatus(), is(404));
+        assertThat("Response Content-Length", response.getField(HttpHeader.CONTENT_LENGTH).getIntValue(), greaterThan(0));
+        assertThat("Response Content-Type", response.get(HttpHeader.CONTENT_TYPE), containsString("text/html;charset=ISO-8859-1"));
+        assertThat(response.getField(HttpHeader.CONNECTION).getValue(), is("close"));
+        assertContent(response);
+    }
+
     @Test
     public void testMoreSpecificAccept() throws Exception
     {