From 3bb7f4e2c407be65323150fe769613ae2edcbf9e Mon Sep 17 00:00:00 2001 From: Cesar Blum Silveira Date: Thu, 30 Jun 2016 15:48:32 -0700 Subject: [PATCH] Treat ECONNRESET as a connection error (#934). --- .../Internal/Http/Connection.cs | 9 +- .../Internal/Http/FrameOfT.cs | 4 +- .../Internal/Http/MessageBody.cs | 12 +- .../Internal/Http/SocketInput.cs | 67 ++++++--- .../Internal/Http/SocketInputExtensions.cs | 4 +- .../Internal/Infrastructure/Constants.cs | 18 --- .../KestrelServer.cs | 4 - .../RequestTests.cs | 139 +++++++++++++++++- .../project.json | 2 +- .../ConnectionTests.cs | 2 +- .../TestInput.cs | 2 +- 11 files changed, 202 insertions(+), 61 deletions(-) diff --git a/src/Microsoft.AspNetCore.Server.Kestrel/Internal/Http/Connection.cs b/src/Microsoft.AspNetCore.Server.Kestrel/Internal/Http/Connection.cs index f3eeac1bdd..c16ef2e652 100644 --- a/src/Microsoft.AspNetCore.Server.Kestrel/Internal/Http/Connection.cs +++ b/src/Microsoft.AspNetCore.Server.Kestrel/Internal/Http/Connection.cs @@ -282,7 +282,7 @@ namespace Microsoft.AspNetCore.Server.Kestrel.Internal.Http } var normalRead = status > 0; - var normalDone = status == Constants.ECONNRESET || status == Constants.EOF; + var normalDone = status == Constants.EOF; var errorDone = !(normalDone || normalRead); var readCount = normalRead ? status : 0; @@ -293,13 +293,18 @@ namespace Microsoft.AspNetCore.Server.Kestrel.Internal.Http else { _socket.ReadStop(); - Log.ConnectionReadFin(ConnectionId); + + if (normalDone) + { + Log.ConnectionReadFin(ConnectionId); + } } Exception error = null; if (errorDone) { handle.Libuv.Check(status, out error); + Log.ConnectionError(ConnectionId, error); } _rawSocketInput.IncomingComplete(readCount, error); diff --git a/src/Microsoft.AspNetCore.Server.Kestrel/Internal/Http/FrameOfT.cs b/src/Microsoft.AspNetCore.Server.Kestrel/Internal/Http/FrameOfT.cs index f19f287075..ae77ca679b 100644 --- a/src/Microsoft.AspNetCore.Server.Kestrel/Internal/Http/FrameOfT.cs +++ b/src/Microsoft.AspNetCore.Server.Kestrel/Internal/Http/FrameOfT.cs @@ -34,7 +34,7 @@ namespace Microsoft.AspNetCore.Server.Kestrel.Internal.Http { while (!_requestProcessingStopping && TakeStartLine(SocketInput) != RequestLineStatus.Done) { - if (SocketInput.RemoteIntakeFin) + if (SocketInput.CheckFinOrThrow()) { // We need to attempt to consume start lines and headers even after // SocketInput.RemoteIntakeFin is set to true to ensure we don't close a @@ -62,7 +62,7 @@ namespace Microsoft.AspNetCore.Server.Kestrel.Internal.Http while (!_requestProcessingStopping && !TakeMessageHeaders(SocketInput, FrameRequestHeaders)) { - if (SocketInput.RemoteIntakeFin) + if (SocketInput.CheckFinOrThrow()) { // We need to attempt to consume start lines and headers even after // SocketInput.RemoteIntakeFin is set to true to ensure we don't close a diff --git a/src/Microsoft.AspNetCore.Server.Kestrel/Internal/Http/MessageBody.cs b/src/Microsoft.AspNetCore.Server.Kestrel/Internal/Http/MessageBody.cs index 5d0e37c29e..a96119cb58 100644 --- a/src/Microsoft.AspNetCore.Server.Kestrel/Internal/Http/MessageBody.cs +++ b/src/Microsoft.AspNetCore.Server.Kestrel/Internal/Http/MessageBody.cs @@ -239,7 +239,7 @@ namespace Microsoft.AspNetCore.Server.Kestrel.Internal.Http { while (_mode == Mode.Prefix) { - var fin = input.RemoteIntakeFin; + var fin = input.CheckFinOrThrow(); ParseChunkedPrefix(input); @@ -257,7 +257,7 @@ namespace Microsoft.AspNetCore.Server.Kestrel.Internal.Http while (_mode == Mode.Extension) { - var fin = input.RemoteIntakeFin; + var fin = input.CheckFinOrThrow(); ParseExtension(input); @@ -275,7 +275,7 @@ namespace Microsoft.AspNetCore.Server.Kestrel.Internal.Http while (_mode == Mode.Data) { - var fin = input.RemoteIntakeFin; + var fin = input.CheckFinOrThrow(); int actual = ReadChunkedData(input, buffer.Array, buffer.Offset, buffer.Count); @@ -297,7 +297,7 @@ namespace Microsoft.AspNetCore.Server.Kestrel.Internal.Http while (_mode == Mode.Suffix) { - var fin = input.RemoteIntakeFin; + var fin = input.CheckFinOrThrow(); ParseChunkedSuffix(input); @@ -317,7 +317,7 @@ namespace Microsoft.AspNetCore.Server.Kestrel.Internal.Http // Chunks finished, parse trailers while (_mode == Mode.Trailer) { - var fin = input.RemoteIntakeFin; + var fin = input.CheckFinOrThrow(); ParseChunkedTrailer(input); @@ -337,7 +337,7 @@ namespace Microsoft.AspNetCore.Server.Kestrel.Internal.Http { while (!_context.TakeMessageHeaders(input, _requestHeaders)) { - if (input.RemoteIntakeFin) + if (input.CheckFinOrThrow()) { if (_context.TakeMessageHeaders(input, _requestHeaders)) { diff --git a/src/Microsoft.AspNetCore.Server.Kestrel/Internal/Http/SocketInput.cs b/src/Microsoft.AspNetCore.Server.Kestrel/Internal/Http/SocketInput.cs index e093df9c8a..dae06a5ce0 100644 --- a/src/Microsoft.AspNetCore.Server.Kestrel/Internal/Http/SocketInput.cs +++ b/src/Microsoft.AspNetCore.Server.Kestrel/Internal/Http/SocketInput.cs @@ -22,7 +22,6 @@ namespace Microsoft.AspNetCore.Server.Kestrel.Internal.Http private readonly ManualResetEventSlim _manualResetEvent = new ManualResetEventSlim(false, 0); private Action _awaitableState; - private Exception _awaitableError; private MemoryPoolBlock _head; private MemoryPoolBlock _tail; @@ -33,6 +32,8 @@ namespace Microsoft.AspNetCore.Server.Kestrel.Internal.Http private bool _consuming; private bool _disposed; + private TaskCompletionSource _tcs = new TaskCompletionSource(); + public SocketInput(MemoryPool memory, IThreadPool threadPool, IBufferSizeControl bufferSizeControl = null) { _memory = memory; @@ -41,10 +42,16 @@ namespace Microsoft.AspNetCore.Server.Kestrel.Internal.Http _awaitableState = _awaitableIsNotCompleted; } - public bool RemoteIntakeFin { get; set; } - public bool IsCompleted => ReferenceEquals(_awaitableState, _awaitableIsCompleted); + private bool ReadingInput => _tcs.Task.Status == TaskStatus.WaitingForActivation; + + public bool CheckFinOrThrow() + { + CheckConnectionError(); + return _tcs.Task.Status == TaskStatus.RanToCompletion; + } + public MemoryPoolBlock IncomingStart() { const int minimumSize = 2048; @@ -87,7 +94,7 @@ namespace Microsoft.AspNetCore.Server.Kestrel.Internal.Http } else { - RemoteIntakeFin = true; + FinReceived(); } Complete(); @@ -122,13 +129,13 @@ namespace Microsoft.AspNetCore.Server.Kestrel.Internal.Http _pinned = null; } - if (count == 0) - { - RemoteIntakeFin = true; - } if (error != null) { - _awaitableError = error; + SetConnectionError(error); + } + else if (count == 0) + { + FinReceived(); } Complete(); @@ -216,8 +223,7 @@ namespace Microsoft.AspNetCore.Server.Kestrel.Internal.Http if (!examined.IsDefault && examined.IsEnd && - RemoteIntakeFin == false && - _awaitableError == null) + ReadingInput) { _manualResetEvent.Reset(); @@ -252,8 +258,7 @@ namespace Microsoft.AspNetCore.Server.Kestrel.Internal.Http public void AbortAwaiting() { - _awaitableError = new TaskCanceledException("The request was aborted"); - + SetConnectionError(new TaskCanceledException("The request was aborted")); Complete(); } @@ -279,7 +284,7 @@ namespace Microsoft.AspNetCore.Server.Kestrel.Internal.Http } else { - _awaitableError = new InvalidOperationException("Concurrent reads are not supported."); + SetConnectionError(new InvalidOperationException("Concurrent reads are not supported.")); Interlocked.Exchange( ref _awaitableState, @@ -303,15 +308,8 @@ namespace Microsoft.AspNetCore.Server.Kestrel.Internal.Http { _manualResetEvent.Wait(); } - var error = _awaitableError; - if (error != null) - { - if (error is TaskCanceledException || error is InvalidOperationException) - { - throw error; - } - throw new IOException(error.Message, error); - } + + CheckConnectionError(); } public void Dispose() @@ -340,5 +338,28 @@ namespace Microsoft.AspNetCore.Server.Kestrel.Internal.Http returnBlock.Pool.Return(returnBlock); } } + + private void SetConnectionError(Exception error) + { + _tcs.TrySetException(error); + } + + private void FinReceived() + { + _tcs.TrySetResult(null); + } + + private void CheckConnectionError() + { + var error = _tcs.Task.Exception?.InnerException; + if (error != null) + { + if (error is TaskCanceledException || error is InvalidOperationException) + { + throw error; + } + throw new IOException(error.Message, error); + } + } } } diff --git a/src/Microsoft.AspNetCore.Server.Kestrel/Internal/Http/SocketInputExtensions.cs b/src/Microsoft.AspNetCore.Server.Kestrel/Internal/Http/SocketInputExtensions.cs index af16744bf5..2ff1f4c037 100644 --- a/src/Microsoft.AspNetCore.Server.Kestrel/Internal/Http/SocketInputExtensions.cs +++ b/src/Microsoft.AspNetCore.Server.Kestrel/Internal/Http/SocketInputExtensions.cs @@ -11,7 +11,7 @@ namespace Microsoft.AspNetCore.Server.Kestrel.Internal.Http { while (input.IsCompleted) { - var fin = input.RemoteIntakeFin; + var fin = input.CheckFinOrThrow(); var begin = input.ConsumingStart(); int actual; @@ -37,7 +37,7 @@ namespace Microsoft.AspNetCore.Server.Kestrel.Internal.Http { await input; - var fin = input.RemoteIntakeFin; + var fin = input.CheckFinOrThrow(); var begin = input.ConsumingStart(); int actual; diff --git a/src/Microsoft.AspNetCore.Server.Kestrel/Internal/Infrastructure/Constants.cs b/src/Microsoft.AspNetCore.Server.Kestrel/Internal/Infrastructure/Constants.cs index 2d270d9955..90f228de31 100644 --- a/src/Microsoft.AspNetCore.Server.Kestrel/Internal/Infrastructure/Constants.cs +++ b/src/Microsoft.AspNetCore.Server.Kestrel/Internal/Infrastructure/Constants.cs @@ -10,7 +10,6 @@ namespace Microsoft.AspNetCore.Server.Kestrel.Internal.Infrastructure public const int ListenBacklog = 128; public const int EOF = -4095; - public static readonly int? ECONNRESET = GetECONNRESET(); public static readonly int? EADDRINUSE = GetEADDRINUSE(); /// @@ -26,23 +25,6 @@ namespace Microsoft.AspNetCore.Server.Kestrel.Internal.Infrastructure public const string ServerName = "Kestrel"; - private static int? GetECONNRESET() - { - if (RuntimeInformation.IsOSPlatform(OSPlatform.Windows)) - { - return -4077; - } - else if (RuntimeInformation.IsOSPlatform(OSPlatform.Linux)) - { - return -104; - } - else if (RuntimeInformation.IsOSPlatform(OSPlatform.OSX)) - { - return -54; - } - return null; - } - private static int? GetEADDRINUSE() { if (RuntimeInformation.IsOSPlatform(OSPlatform.Windows)) diff --git a/src/Microsoft.AspNetCore.Server.Kestrel/KestrelServer.cs b/src/Microsoft.AspNetCore.Server.Kestrel/KestrelServer.cs index 3a38ce6df6..034d41e29e 100644 --- a/src/Microsoft.AspNetCore.Server.Kestrel/KestrelServer.cs +++ b/src/Microsoft.AspNetCore.Server.Kestrel/KestrelServer.cs @@ -92,10 +92,6 @@ namespace Microsoft.AspNetCore.Server.Kestrel "ThreadCount must be positive."); } - if (!Constants.ECONNRESET.HasValue) - { - _logger.LogWarning("Unable to determine ECONNRESET value on this platform."); - } if (!Constants.EADDRINUSE.HasValue) { _logger.LogWarning("Unable to determine EADDRINUSE value on this platform."); diff --git a/test/Microsoft.AspNetCore.Server.Kestrel.FunctionalTests/RequestTests.cs b/test/Microsoft.AspNetCore.Server.Kestrel.FunctionalTests/RequestTests.cs index a01b84f7b8..4459423a3d 100644 --- a/test/Microsoft.AspNetCore.Server.Kestrel.FunctionalTests/RequestTests.cs +++ b/test/Microsoft.AspNetCore.Server.Kestrel.FunctionalTests/RequestTests.cs @@ -2,17 +2,23 @@ // Licensed under the Apache License, Version 2.0. See License.txt in the project root for license information. using System; +using System.Collections.Generic; using System.Globalization; using System.IO; +using System.Linq; using System.Net; using System.Net.Http; using System.Net.Sockets; using System.Text; +using System.Threading; using System.Threading.Tasks; using Microsoft.AspNetCore.Builder; using Microsoft.AspNetCore.Hosting; using Microsoft.AspNetCore.Http; +using Microsoft.AspNetCore.Server.Kestrel.Internal.Networking; +using Microsoft.AspNetCore.Testing; using Microsoft.AspNetCore.Testing.xunit; +using Microsoft.Extensions.Logging.Testing; using Newtonsoft.Json; using Newtonsoft.Json.Linq; using Xunit; @@ -184,6 +190,102 @@ namespace Microsoft.AspNetCore.Server.Kestrel.FunctionalTests } } + [Fact] + public async Task ConnectionResetAbortsRequest() + { + var connectionErrorLogged = new SemaphoreSlim(0); + var testSink = new ConnectionErrorTestSink(() => connectionErrorLogged.Release()); + var builder = new WebHostBuilder() + .UseLoggerFactory(new TestLoggerFactory(testSink, true)) + .UseKestrel() + .UseUrls($"http://127.0.0.1:0") + .Configure(app => app.Run(context => + { + return Task.FromResult(0); + })); + + using (var host = builder.Build()) + { + host.Start(); + + using (var socket = new Socket(AddressFamily.InterNetwork, SocketType.Stream, ProtocolType.Tcp)) + { + socket.Connect(new IPEndPoint(IPAddress.Loopback, host.GetPort())); + socket.LingerState = new LingerOption(true, 0); + } + + // Wait until connection error is logged + Assert.True(await connectionErrorLogged.WaitAsync(2500)); + + // Check for expected message + Assert.NotNull(testSink.ConnectionErrorMessage); + Assert.Contains("ECONNRESET", testSink.ConnectionErrorMessage); + } + } + + [Fact] + public async Task ThrowsOnReadAfterConnectionError() + { + var requestStarted = new SemaphoreSlim(0); + var connectionReset = new SemaphoreSlim(0); + var appDone = new SemaphoreSlim(0); + var expectedExceptionThrown = false; + + var builder = new WebHostBuilder() + .UseKestrel() + .UseUrls($"http://127.0.0.1:0") + .Configure(app => app.Run(async context => + { + requestStarted.Release(); + Assert.True(await connectionReset.WaitAsync(2500)); + + try + { + await context.Request.Body.ReadAsync(new byte[1], 0, 1); + } + catch (BadHttpRequestException) + { + // We need this here because BadHttpRequestException derives from IOException, + // and we're looking for an actual IOException. + } + catch (IOException ex) + { + // This is one of two exception types that might thrown in this scenario. + // An IOException is thrown if ReadAsync is awaiting on SocketInput when + // the connection is aborted. + expectedExceptionThrown = ex.InnerException is UvException; + } + catch (TaskCanceledException) + { + // This is the other exception type that might be thrown here. + // A TaskCanceledException is thrown if ReadAsync is called when the + // connection has already been aborted, since FrameRequestStream is + // aborted and returns a canceled task from ReadAsync. + expectedExceptionThrown = true; + } + + appDone.Release(); + })); + + using (var host = builder.Build()) + { + host.Start(); + + using (var socket = new Socket(AddressFamily.InterNetwork, SocketType.Stream, ProtocolType.Tcp)) + { + socket.Connect(new IPEndPoint(IPAddress.Loopback, host.GetPort())); + socket.LingerState = new LingerOption(true, 0); + socket.Send(Encoding.ASCII.GetBytes("GET / HTTP/1.1\r\nContent-Length: 1\r\n\r\n")); + Assert.True(await requestStarted.WaitAsync(2500)); + } + + connectionReset.Release(); + + Assert.True(await appDone.WaitAsync(2500)); + Assert.True(expectedExceptionThrown); + } + } + private async Task TestRemoteIPAddress(string registerAddress, string requestAddress, string expectAddress) { var builder = new WebHostBuilder() @@ -220,5 +322,40 @@ namespace Microsoft.AspNetCore.Server.Kestrel.FunctionalTests Assert.NotEmpty(facts["RemotePort"].Value()); } } + + private class ConnectionErrorTestSink : ITestSink + { + private readonly Action _connectionErrorLogged; + + public ConnectionErrorTestSink(Action connectionErrorLogged) + { + _connectionErrorLogged = connectionErrorLogged; + } + + public string ConnectionErrorMessage { get; set; } + + public Func BeginEnabled { get; set; } + + public List Scopes { get; set; } + + public Func WriteEnabled { get; set; } + + public List Writes { get; set; } + + public void Begin(BeginScopeContext context) + { + } + + public void Write(WriteContext context) + { + const int connectionErrorEventId = 14; + + if (context.EventId.Id == connectionErrorEventId) + { + ConnectionErrorMessage = context.Exception?.Message; + _connectionErrorLogged(); + } + } + } } -} +} \ No newline at end of file diff --git a/test/Microsoft.AspNetCore.Server.Kestrel.FunctionalTests/project.json b/test/Microsoft.AspNetCore.Server.Kestrel.FunctionalTests/project.json index 9b247ae340..c2d82b3f4c 100644 --- a/test/Microsoft.AspNetCore.Server.Kestrel.FunctionalTests/project.json +++ b/test/Microsoft.AspNetCore.Server.Kestrel.FunctionalTests/project.json @@ -6,7 +6,7 @@ "Microsoft.AspNetCore.Server.Kestrel": "1.1.0-*", "Microsoft.AspNetCore.Server.Kestrel.Https": "1.1.0-*", "Microsoft.AspNetCore.Testing": "1.1.0-*", - "Microsoft.Extensions.Logging.Console": "1.1.0-*", + "Microsoft.Extensions.Logging.Testing": "1.1.0-*", "Newtonsoft.Json": "9.0.1", "xunit": "2.2.0-*" }, diff --git a/test/Microsoft.AspNetCore.Server.KestrelTests/ConnectionTests.cs b/test/Microsoft.AspNetCore.Server.KestrelTests/ConnectionTests.cs index f6b43c2d13..261bb47068 100644 --- a/test/Microsoft.AspNetCore.Server.KestrelTests/ConnectionTests.cs +++ b/test/Microsoft.AspNetCore.Server.KestrelTests/ConnectionTests.cs @@ -38,7 +38,7 @@ namespace Microsoft.AspNetCore.Server.KestrelTests Libuv.uv_buf_t ignored; mockLibuv.AllocCallback(socket.InternalGetHandle(), 2048, out ignored); mockLibuv.ReadCallback(socket.InternalGetHandle(), 0, ref ignored); - Assert.False(connection.SocketInput.RemoteIntakeFin); + Assert.False(connection.SocketInput.CheckFinOrThrow()); connection.ConnectionControl.End(ProduceEndType.SocketDisconnect); } diff --git a/test/Microsoft.AspNetCore.Server.KestrelTests/TestInput.cs b/test/Microsoft.AspNetCore.Server.KestrelTests/TestInput.cs index a868068b5c..08106ed1c0 100644 --- a/test/Microsoft.AspNetCore.Server.KestrelTests/TestInput.cs +++ b/test/Microsoft.AspNetCore.Server.KestrelTests/TestInput.cs @@ -40,7 +40,7 @@ namespace Microsoft.AspNetCore.Server.KestrelTests FrameContext.SocketInput.IncomingData(data, 0, data.Length); if (fin) { - FrameContext.SocketInput.RemoteIntakeFin = true; + FrameContext.SocketInput.IncomingFin(); } }