From 30010a103bb20f913d00f64d49a9914d37eb8650 Mon Sep 17 00:00:00 2001 From: Andrew Stanton-Nurse Date: Mon, 23 Oct 2017 10:52:02 -0700 Subject: [PATCH] Fix for #2085 - "The Detaskening" (#2123) --- .../Internal/Http/Http1Connection.cs | 116 ++++++++++-------- .../Internal/Http/HttpProtocol.cs | 28 ++++- .../Internal/Http2/Http2Stream.cs | 8 +- 3 files changed, 96 insertions(+), 56 deletions(-) diff --git a/src/Kestrel.Core/Internal/Http/Http1Connection.cs b/src/Kestrel.Core/Internal/Http/Http1Connection.cs index 9357a9d742..b5d9c0235a 100644 --- a/src/Kestrel.Core/Internal/Http/Http1Connection.cs +++ b/src/Kestrel.Core/Internal/Http/Http1Connection.cs @@ -413,67 +413,81 @@ namespace Microsoft.AspNetCore.Server.Kestrel.Core.Internal.Http protected override MessageBody CreateMessageBody() => Http1MessageBody.For(_httpVersion, HttpRequestHeaders, this); - protected override async Task ParseRequestAsync() + protected override void BeginRequestProcessing() { + // Reset the features and timeout. Reset(); TimeoutControl.SetTimeout(_keepAliveTicks, TimeoutAction.StopProcessingNextRequest); + } - while (_requestProcessingStatus != RequestProcessingStatus.AppStarted) + protected override bool BeginRead(out ReadableBufferAwaitable awaitable) + { + awaitable = Input.ReadAsync(); + return true; + } + + protected override bool TryParseRequest(ReadResult result, out bool endConnection) + { + var examined = result.Buffer.End; + var consumed = result.Buffer.End; + + try { - var result = await Input.ReadAsync(); - - var examined = result.Buffer.End; - var consumed = result.Buffer.End; - - try + ParseRequest(result.Buffer, out consumed, out examined); + } + catch (InvalidOperationException) + { + if (_requestProcessingStatus == RequestProcessingStatus.ParsingHeaders) { - ParseRequest(result.Buffer, out consumed, out examined); - } - catch (InvalidOperationException) - { - if (_requestProcessingStatus == RequestProcessingStatus.ParsingHeaders) - { - throw BadHttpRequestException.GetException(RequestRejectionReason - .MalformedRequestInvalidHeaders); - } - throw; - } - finally - { - Input.Advance(consumed, examined); - } - - if (result.IsCompleted) - { - switch (_requestProcessingStatus) - { - case RequestProcessingStatus.RequestPending: - return false; - case RequestProcessingStatus.ParsingRequestLine: - throw BadHttpRequestException.GetException( - RequestRejectionReason.InvalidRequestLine); - case RequestProcessingStatus.ParsingHeaders: - throw BadHttpRequestException.GetException( - RequestRejectionReason.MalformedRequestInvalidHeaders); - } - } - else if (!_keepAlive && _requestProcessingStatus == RequestProcessingStatus.RequestPending) - { - // Stop the request processing loop if the server is shutting down or there was a keep-alive timeout - // and there is no ongoing request. - return false; - } - else if (RequestTimedOut) - { - // In this case, there is an ongoing request but the start line/header parsing has timed out, so send - // a 408 response. - throw BadHttpRequestException.GetException(RequestRejectionReason.RequestTimeout); + throw BadHttpRequestException.GetException(RequestRejectionReason + .MalformedRequestInvalidHeaders); } + throw; + } + finally + { + Input.Advance(consumed, examined); } - EnsureHostHeaderExists(); + if (result.IsCompleted) + { + switch (_requestProcessingStatus) + { + case RequestProcessingStatus.RequestPending: + endConnection = true; + return true; + case RequestProcessingStatus.ParsingRequestLine: + throw BadHttpRequestException.GetException( + RequestRejectionReason.InvalidRequestLine); + case RequestProcessingStatus.ParsingHeaders: + throw BadHttpRequestException.GetException( + RequestRejectionReason.MalformedRequestInvalidHeaders); + } + } + else if (!_keepAlive && _requestProcessingStatus == RequestProcessingStatus.RequestPending) + { + // Stop the request processing loop if the server is shutting down or there was a keep-alive timeout + // and there is no ongoing request. + endConnection = true; + return true; + } + else if (RequestTimedOut) + { + // In this case, there is an ongoing request but the start line/header parsing has timed out, so send + // a 408 response. + throw BadHttpRequestException.GetException(RequestRejectionReason.RequestTimeout); + } - return true; + endConnection = false; + if (_requestProcessingStatus == RequestProcessingStatus.AppStarted) + { + EnsureHostHeaderExists(); + return true; + } + else + { + return false; + } } } } diff --git a/src/Kestrel.Core/Internal/Http/HttpProtocol.cs b/src/Kestrel.Core/Internal/Http/HttpProtocol.cs index 7ac194dd6e..9f6c11740d 100644 --- a/src/Kestrel.Core/Internal/Http/HttpProtocol.cs +++ b/src/Kestrel.Core/Internal/Http/HttpProtocol.cs @@ -375,11 +375,21 @@ namespace Microsoft.AspNetCore.Server.Kestrel.Core.Internal.Http { } + protected virtual void BeginRequestProcessing() + { + } + + protected virtual bool BeginRead(out ReadableBufferAwaitable awaitable) + { + awaitable = default; + return false; + } + protected abstract string CreateRequestId(); protected abstract MessageBody CreateMessageBody(); - protected abstract Task ParseRequestAsync(); + protected abstract bool TryParseRequest(ReadResult result, out bool endConnection); protected abstract void CreateHttpContext(); @@ -436,7 +446,19 @@ namespace Microsoft.AspNetCore.Server.Kestrel.Core.Internal.Http { while (_keepAlive) { - if (!await ParseRequestAsync()) + BeginRequestProcessing(); + + var result = default(ReadResult); + var endConnection = false; + do + { + if (BeginRead(out var awaitable)) + { + result = await awaitable; + } + } while (!TryParseRequest(result, out endConnection)); + + if (endConnection) { return; } @@ -655,7 +677,7 @@ namespace Microsoft.AspNetCore.Server.Kestrel.Core.Internal.Http try { var count = onStarting.Count; - for(var i = 0; i < count; i++) + for (var i = 0; i < count; i++) { var entry = onStarting.Pop(); var task = entry.Key.Invoke(entry.Value); diff --git a/src/Kestrel.Core/Internal/Http2/Http2Stream.cs b/src/Kestrel.Core/Internal/Http2/Http2Stream.cs index c9832ef03d..ea8a564d9f 100644 --- a/src/Kestrel.Core/Internal/Http2/Http2Stream.cs +++ b/src/Kestrel.Core/Internal/Http2/Http2Stream.cs @@ -47,8 +47,11 @@ namespace Microsoft.AspNetCore.Server.Kestrel.Core.Internal.Http2 protected override MessageBody CreateMessageBody() => Http2MessageBody.For(HttpRequestHeaders, this); - protected override Task ParseRequestAsync() + protected override bool TryParseRequest(ReadResult result, out bool endConnection) { + // We don't need any of the parameters because we don't implement BeginRead to actually + // do the reading from a pipeline, nor do we use endConnection to report connection-level errors. + Method = RequestHeaders[":method"]; Scheme = RequestHeaders[":scheme"]; _httpVersion = Http.HttpVersion.Http2; @@ -62,7 +65,8 @@ namespace Microsoft.AspNetCore.Server.Kestrel.Core.Internal.Http2 RequestHeaders["Host"] = RequestHeaders[":authority"]; - return Task.FromResult(true); + endConnection = false; + return true; } public async Task OnDataAsync(ArraySegment data, bool endStream)