diff --git a/src/Kestrel.Core/Internal/Http/Http1MessageBody.cs b/src/Kestrel.Core/Internal/Http/Http1MessageBody.cs index c47bf114b6..d65805cc83 100644 --- a/src/Kestrel.Core/Internal/Http/Http1MessageBody.cs +++ b/src/Kestrel.Core/Internal/Http/Http1MessageBody.cs @@ -89,6 +89,14 @@ namespace Microsoft.AspNetCore.Server.Kestrel.Core.Internal.Http } else if (result.IsCompleted) { + // Treat any FIN from an upgraded request as expected. + // It's up to higher-level consumer (i.e. WebSocket middleware) to determine + // if the end is actually expected based on higher-level framing. + if (RequestUpgrade) + { + break; + } + BadHttpRequestException.Throw(RequestRejectionReason.UnexpectedEndOfRequestContent); } diff --git a/src/Kestrel.Core/Internal/Http/HttpProtocol.FeatureCollection.cs b/src/Kestrel.Core/Internal/Http/HttpProtocol.FeatureCollection.cs index 055e8a1e76..3c53523f8a 100644 --- a/src/Kestrel.Core/Internal/Http/HttpProtocol.FeatureCollection.cs +++ b/src/Kestrel.Core/Internal/Http/HttpProtocol.FeatureCollection.cs @@ -8,6 +8,7 @@ using System.IO; using System.Net; using System.Threading; using System.Threading.Tasks; +using Microsoft.AspNetCore.Connections; using Microsoft.AspNetCore.Http; using Microsoft.AspNetCore.Http.Features; using Microsoft.AspNetCore.Server.Kestrel.Core.Features; @@ -282,7 +283,7 @@ namespace Microsoft.AspNetCore.Server.Kestrel.Core.Internal.Http void IHttpRequestLifetimeFeature.Abort() { - Abort(error: null); + Abort(new ConnectionAbortedException()); } } } diff --git a/src/Kestrel.Core/Internal/Http/HttpProtocol.cs b/src/Kestrel.Core/Internal/Http/HttpProtocol.cs index 7db0062a15..95b716abf9 100644 --- a/src/Kestrel.Core/Internal/Http/HttpProtocol.cs +++ b/src/Kestrel.Core/Internal/Http/HttpProtocol.cs @@ -411,21 +411,29 @@ namespace Microsoft.AspNetCore.Server.Kestrel.Core.Internal.Http } /// - /// Immediate kill the connection and poison the request and response streams. + /// Immediately kill the connection and poison the request and response streams with an error if there is one. /// public void Abort(Exception error) { - if (Interlocked.Exchange(ref _requestAborted, 1) == 0) + if (Interlocked.Exchange(ref _requestAborted, 1) != 0) { - _keepAlive = false; - - _streams?.Abort(error); - - Output.Abort(error); - - // Potentially calling user code. CancelRequestAbortedToken logs any exceptions. - ServiceContext.Scheduler.Schedule(state => ((HttpProtocol)state).CancelRequestAbortedToken(), this); + return; } + + _keepAlive = false; + + // If Abort() isn't called with an exception, there was a FIN. In this case, even though the connection is + // still closed immediately, we allow the app to drain the data in the request buffer. If the request data + // was truncated, MessageBody will complete the RequestBodyPipe with an error. + if (error != null) + { + _streams?.Abort(error); + } + + Output.Abort(error); + + // Potentially calling user code. CancelRequestAbortedToken logs any exceptions. + ServiceContext.Scheduler.Schedule(state => ((HttpProtocol)state).CancelRequestAbortedToken(), this); } public void OnHeader(Span name, Span value) diff --git a/test/Kestrel.Core.Tests/MessageBodyTests.cs b/test/Kestrel.Core.Tests/MessageBodyTests.cs index fb2d8613db..f472269e6c 100644 --- a/test/Kestrel.Core.Tests/MessageBodyTests.cs +++ b/test/Kestrel.Core.Tests/MessageBodyTests.cs @@ -786,7 +786,7 @@ namespace Microsoft.AspNetCore.Server.Kestrel.Core.Tests input.Fin(); - await Assert.ThrowsAsync(async () => await body.ReadAsync(new ArraySegment(new byte[1]))); + Assert.Equal(0, await body.ReadAsync(new ArraySegment(new byte[1]))); mockTimeoutControl.Verify(timeoutControl => timeoutControl.StartTimingReads(), Times.Never); mockTimeoutControl.Verify(timeoutControl => timeoutControl.StopTimingReads(), Times.Never); diff --git a/test/Kestrel.FunctionalTests/UpgradeTests.cs b/test/Kestrel.FunctionalTests/UpgradeTests.cs index 5d89707335..6a839aaad3 100644 --- a/test/Kestrel.FunctionalTests/UpgradeTests.cs +++ b/test/Kestrel.FunctionalTests/UpgradeTests.cs @@ -297,5 +297,42 @@ namespace Microsoft.AspNetCore.Server.Kestrel.FunctionalTests var exception = await Assert.ThrowsAsync(async () => await upgradeTcs.Task.TimeoutAfter(TimeSpan.FromSeconds(60))); Assert.Equal(CoreStrings.UpgradedConnectionLimitReached, exception.Message); } + + [Fact] + public async Task DoesNotThrowOnFin() + { + var appCompletedTcs = new TaskCompletionSource(TaskCreationOptions.RunContinuationsAsynchronously); + + using (var server = new TestServer(async context => + { + var feature = context.Features.Get(); + var duplexStream = await feature.UpgradeAsync(); + + try + { + await duplexStream.CopyToAsync(Stream.Null); + appCompletedTcs.SetResult(null); + } + catch (Exception ex) + { + appCompletedTcs.SetException(ex); + throw; + } + + }, new TestServiceContext(LoggerFactory))) + { + using (var connection = server.CreateConnection()) + { + await connection.SendEmptyGetWithUpgrade(); + await connection.Receive("HTTP/1.1 101 Switching Protocols", + "Connection: Upgrade", + $"Date: {server.Context.DateHeaderValue}", + "", + ""); + } + + await appCompletedTcs.Task.TimeoutAfter(TestConstants.DefaultTimeout); + } + } } }