Separate request rejection from bad request state setting.

src/Microsoft.AspNetCore.Server.Kestrel/Internal/Http/Frame.cs

@@ -47,7 +47,6 @@ namespace Microsoft.AspNetCore.Server.Kestrel.Internal.Http
         private readonly object _onStartingSync = new Object();
         private readonly object _onCompletedSync = new Object();
 
-        protected bool _requestRejected;
         private Streams _frameStreams;
 
         protected Stack<KeyValuePair<Func<object, Task>, object>> _onStarting;
@@ -64,6 +63,7 @@ namespace Microsoft.AspNetCore.Server.Kestrel.Internal.Http
         private bool _canHaveBody;
         private bool _autoChunk;
         protected Exception _applicationException;
+        private BadHttpRequestException _requestRejectedException;
 
         protected HttpVersion _httpVersion;
 
@@ -717,7 +717,7 @@ namespace Microsoft.AspNetCore.Server.Kestrel.Internal.Http
 
         protected Task TryProduceInvalidRequestResponse()
         {
-            if (_requestRejected)
+            if (_requestRejectedException != null)
             {
                 if (FrameRequestHeaders == null || FrameResponseHeaders == null)
                 {
@@ -732,7 +732,7 @@ namespace Microsoft.AspNetCore.Server.Kestrel.Internal.Http
 
         protected Task ProduceEnd()
         {
-            if (_requestRejected || _applicationException != null)
+            if (_requestRejectedException != null || _applicationException != null)
             {
                 if (HasResponseStarted)
                 {
@@ -741,8 +741,13 @@ namespace Microsoft.AspNetCore.Server.Kestrel.Internal.Http
                     return TaskCache.CompletedTask;
                 }
 
-                // If the request was rejected, the error state has already been set by SetBadRequestState
+                // If the request was rejected, the error state has already been set by SetBadRequestState and
-                if (!_requestRejected)
+                // that should take precedence.
+                if (_requestRejectedException != null)
+                {
+                    SetErrorResponseHeaders(statusCode: _requestRejectedException.StatusCode);
+                }
+                else
                 {
                     // 500 Internal Server Error
                     SetErrorResponseHeaders(statusCode: 500);
@@ -1394,24 +1399,19 @@ namespace Microsoft.AspNetCore.Server.Kestrel.Internal.Http
                     _applicationException);
         }
 
-        public void RejectRequest(string message)
-        {
-            throw new ObjectDisposedException(
-                "The response has been aborted due to an unhandled application exception.",
-                _applicationException);
-        }
-
         public void RejectRequest(RequestRejectionReason reason)
         {
-            var ex = BadHttpRequestException.GetException(reason);
+            RejectRequest(BadHttpRequestException.GetException(reason));
-            SetBadRequestState(ex);
-            throw ex;
         }
 
         public void RejectRequest(RequestRejectionReason reason, string value)
         {
-            var ex = BadHttpRequestException.GetException(reason, value);
+            RejectRequest(BadHttpRequestException.GetException(reason, value));
-            SetBadRequestState(ex);
+        }
+
+        private void RejectRequest(BadHttpRequestException ex)
+        {
+            Log.ConnectionBadRequest(ConnectionId, ex);
             throw ex;
         }
 
@@ -1422,17 +1422,14 @@ namespace Microsoft.AspNetCore.Server.Kestrel.Internal.Http
 
         public void SetBadRequestState(BadHttpRequestException ex)
         {
-            // Setting status code will throw if response has already started
             if (!HasResponseStarted)
             {
-                SetErrorResponseHeaders(statusCode: ex.StatusCode);
+                SetErrorResponseHeaders(ex.StatusCode);
             }
 
             _keepAlive = false;
             _requestProcessingStopping = true;
-            _requestRejected = true;
+            _requestRejectedException = ex;
-
-            Log.ConnectionBadRequest(ConnectionId, ex);
         }
 
         protected void ReportApplicationError(Exception ex)

src/Microsoft.AspNetCore.Server.Kestrel/Internal/Http/FrameOfT.cs

@@ -100,6 +100,11 @@ namespace Microsoft.AspNetCore.Server.Kestrel.Internal.Http
                             catch (Exception ex)
                             {
                                 ReportApplicationError(ex);
+
+                                if (ex is BadHttpRequestException)
+                                {
+                                    throw;
+                                }
                             }
                             finally
                             {
@@ -118,30 +123,37 @@ namespace Microsoft.AspNetCore.Server.Kestrel.Internal.Http
                                 {
                                     await FireOnCompleted();
                                 }
-
-                                // If _requestAbort is set, the connection has already been closed.
-                                if (Volatile.Read(ref _requestAborted) == 0)
-                                {
-                                    ResumeStreams();
-
-                                    if (_keepAlive)
-                                    {
-                                        // Finish reading the request body in case the app did not.
-                                        await messageBody.Consume();
-                                    }
-
-                                    // ProduceEnd() must be called before _application.DisposeContext(), to ensure
-                                    // HttpContext.Response.StatusCode is correctly set when
-                                    // IHttpContextFactory.Dispose(HttpContext) is called.
-                                    await ProduceEnd();
-                                }
-                                else if (!HasResponseStarted)
-                                {
-                                    // If the request was aborted and no response was sent, there's no
-                                    // meaningful status code to log.
-                                    StatusCode = 0;
-                                }
                             }
+
+                            // If _requestAbort is set, the connection has already been closed.
+                            if (Volatile.Read(ref _requestAborted) == 0)
+                            {
+                                ResumeStreams();
+
+                                if (_keepAlive)
+                                {
+                                    // Finish reading the request body in case the app did not.
+                                    await messageBody.Consume();
+                                }
+
+                                // ProduceEnd() must be called before _application.DisposeContext(), to ensure
+                                // HttpContext.Response.StatusCode is correctly set when
+                                // IHttpContextFactory.Dispose(HttpContext) is called.
+                                await ProduceEnd();
+                            }
+                            else if (!HasResponseStarted)
+                            {
+                                // If the request was aborted and no response was sent, there's no
+                                // meaningful status code to log.
+                                StatusCode = 0;
+                            }
+                        }
+                        catch (BadHttpRequestException ex)
+                        {
+                            // Handle BadHttpRequestException thrown during app execution or remaining message body consumption.
+                            // This has to be caught here so StatusCode is set properly before disposing the HttpContext
+                            // (DisposeContext logs StatusCode).
+                            SetBadRequestState(ex);
                         }
                         finally
                         {
@@ -169,11 +181,9 @@ namespace Microsoft.AspNetCore.Server.Kestrel.Internal.Http
             }
             catch (BadHttpRequestException ex)
             {
-                if (!_requestRejected)
+                // Handle BadHttpRequestException thrown during request line or header parsing.
-                {
+                // SetBadRequestState logs the error.
-                    // SetBadRequestState logs the error.
+                SetBadRequestState(ex);
-                    SetBadRequestState(ex);
-                }
             }
             catch (Exception ex)
             {
@@ -183,11 +193,10 @@ namespace Microsoft.AspNetCore.Server.Kestrel.Internal.Http
             {
                 try
                 {
-                    await TryProduceInvalidRequestResponse();
-
                     // If _requestAborted is set, the connection has already been closed.
                     if (Volatile.Read(ref _requestAborted) == 0)
                     {
+                        await TryProduceInvalidRequestResponse();
                         ConnectionControl.End(ProduceEndType.SocketShutdown);
                     }
                 }

test/Microsoft.AspNetCore.Server.Kestrel.FunctionalTests/ResponseTests.cs

@@ -2,7 +2,6 @@
 // Licensed under the Apache License, Version 2.0. See License.txt in the project root for license information.
 
 using System;
-using System.IO;
 using System.Linq;
 using System.Net;
 using System.Net.Http;
@@ -239,6 +238,38 @@ namespace Microsoft.AspNetCore.Server.Kestrel.FunctionalTests
                 sendMalformedRequest: true);
         }
 
+        [Fact]
+        public Task ResponseStatusCodeSetBeforeHttpContextDisposedRequestMalformedRead()
+        {
+            return ResponseStatusCodeSetBeforeHttpContextDispose(
+                async context =>
+                {
+                    await context.Request.Body.ReadAsync(new byte[1], 0, 1);
+                },
+                expectedClientStatusCode: null,
+                expectedServerStatusCode: HttpStatusCode.BadRequest,
+                sendMalformedRequest: true);
+        }
+
+        [Fact]
+        public Task ResponseStatusCodeSetBeforeHttpContextDisposedRequestMalformedReadIgnored()
+        {
+            return ResponseStatusCodeSetBeforeHttpContextDispose(
+                async context =>
+                {
+                    try
+                    {
+                        await context.Request.Body.ReadAsync(new byte[1], 0, 1);
+                    }
+                    catch (BadHttpRequestException)
+                    {
+                    }
+                },
+                expectedClientStatusCode: null,
+                expectedServerStatusCode: HttpStatusCode.BadRequest,
+                sendMalformedRequest: true);
+        }
+
         private static async Task ResponseStatusCodeSetBeforeHttpContextDispose(
             RequestDelegate handler,
             HttpStatusCode? expectedClientStatusCode,
@@ -730,14 +761,11 @@ namespace Microsoft.AspNetCore.Server.Kestrel.FunctionalTests
         [Fact]
         public async Task HeadResponseCanContainContentLengthHeader()
         {
-            var testLogger = new TestApplicationErrorLogger();
-            var serviceContext = new TestServiceContext { Log = new TestKestrelTrace(testLogger) };
-
             using (var server = new TestServer(httpContext =>
             {
                 httpContext.Response.ContentLength = 42;
                 return TaskCache.CompletedTask;
-            }, serviceContext))
+            }, new TestServiceContext()))
             {
                 using (var connection = server.CreateConnection())
                 {
@@ -746,7 +774,7 @@ namespace Microsoft.AspNetCore.Server.Kestrel.FunctionalTests
                         "",
                         "");
                     await connection.ReceiveEnd(
-                        $"HTTP/1.1 200 OK",
+                        "HTTP/1.1 200 OK",
                         $"Date: {server.Context.DateHeaderValue}",
                         "Content-Length: 42",
                         "",
@@ -758,14 +786,11 @@ namespace Microsoft.AspNetCore.Server.Kestrel.FunctionalTests
         [Fact]
         public async Task HeadResponseCanContainContentLengthHeaderButBodyNotWritten()
         {
-            var testLogger = new TestApplicationErrorLogger();
-            var serviceContext = new TestServiceContext { Log = new TestKestrelTrace(testLogger) };
-
             using (var server = new TestServer(async httpContext =>
             {
                 httpContext.Response.ContentLength = 12;
                 await httpContext.Response.WriteAsync("hello, world");
-            }, serviceContext))
+            }, new TestServiceContext()))
             {
                 using (var connection = server.CreateConnection())
                 {
@@ -774,7 +799,7 @@ namespace Microsoft.AspNetCore.Server.Kestrel.FunctionalTests
                         "",
                         "");
                     await connection.ReceiveEnd(
-                        $"HTTP/1.1 200 OK",
+                        "HTTP/1.1 200 OK",
                         $"Date: {server.Context.DateHeaderValue}",
                         "Content-Length: 12",
                         "",
@@ -783,6 +808,46 @@ namespace Microsoft.AspNetCore.Server.Kestrel.FunctionalTests
             }
         }
 
+        [Fact]
+        public async Task AppCanWriteOwnBadRequestResponse()
+        {
+            var expectedResponse = string.Empty;
+            var responseWrittenTcs = new TaskCompletionSource<object>();
+
+            using (var server = new TestServer(async httpContext =>
+            {
+                try
+                {
+                    await httpContext.Request.Body.ReadAsync(new byte[1], 0, 1);
+                }
+                catch (BadHttpRequestException ex)
+                {
+                    expectedResponse = ex.Message;
+                    httpContext.Response.StatusCode = 400;
+                    httpContext.Response.ContentLength = ex.Message.Length;
+                    await httpContext.Response.WriteAsync(ex.Message);
+                    responseWrittenTcs.SetResult(null);
+                }
+            }, new TestServiceContext()))
+            {
+                using (var connection = server.CreateConnection())
+                {
+                    await connection.SendEnd(
+                        "POST / HTTP/1.1",
+                        "Transfer-Encoding: chunked",
+                        "",
+                        "bad");
+                    await responseWrittenTcs.Task;
+                    await connection.ReceiveEnd(
+                        "HTTP/1.1 400 Bad Request",
+                        $"Date: {server.Context.DateHeaderValue}",
+                        $"Content-Length: {expectedResponse.Length}",
+                        "",
+                        expectedResponse);
+                }
+            }
+        }
+
         public static TheoryData<string, StringValues, string> NullHeaderData
         {
             get