From 3c0e0f8d889a5d3341a45a1fc5dadbedbead0bb6 Mon Sep 17 00:00:00 2001 From: Cesar Blum Silveira Date: Fri, 15 Jul 2016 16:51:28 -0700 Subject: [PATCH] Always throw same exception type on read after connection error (#975). --- .../Internal/Http/Connection.cs | 15 +++++++++------ .../Internal/Http/Frame.cs | 4 ++-- .../Internal/Http/FrameRequestStream.cs | 11 ++++++++--- .../Internal/Http/SocketInput.cs | 7 +------ .../Internal/Infrastructure/TaskUtilities.cs | 12 ++++++++++++ .../RequestTests.cs | 18 +----------------- .../FrameRequestStreamTests.cs | 12 ++++++++++++ .../TestHelpers/MockConnection.cs | 2 +- 8 files changed, 46 insertions(+), 35 deletions(-) diff --git a/src/Microsoft.AspNetCore.Server.Kestrel/Internal/Http/Connection.cs b/src/Microsoft.AspNetCore.Server.Kestrel/Internal/Http/Connection.cs index da2d7f9bc6..0976a37746 100644 --- a/src/Microsoft.AspNetCore.Server.Kestrel/Internal/Http/Connection.cs +++ b/src/Microsoft.AspNetCore.Server.Kestrel/Internal/Http/Connection.cs @@ -3,6 +3,7 @@ using System; using System.Diagnostics; +using System.IO; using System.Threading; using System.Threading.Tasks; using Microsoft.AspNetCore.Server.Kestrel.Filter; @@ -128,13 +129,13 @@ namespace Microsoft.AspNetCore.Server.Kestrel.Internal.Http return _socketClosedTcs.Task; } - public virtual void Abort() + public virtual void Abort(Exception error = null) { // Frame.Abort calls user code while this method is always // called from a libuv thread. ThreadPool.Run(() => { - _frame.Abort(); + _frame.Abort(error); }); } @@ -231,18 +232,20 @@ namespace Microsoft.AspNetCore.Server.Kestrel.Internal.Http } } - Exception error = null; + IOException error = null; if (errorDone) { - handle.Libuv.Check(status, out error); - Log.ConnectionError(ConnectionId, error); + Exception uvError; + handle.Libuv.Check(status, out uvError); + Log.ConnectionError(ConnectionId, uvError); + error = new IOException(uvError.Message, uvError); } SocketInput.IncomingComplete(readCount, error); if (errorDone) { - Abort(); + Abort(error); } } diff --git a/src/Microsoft.AspNetCore.Server.Kestrel/Internal/Http/Frame.cs b/src/Microsoft.AspNetCore.Server.Kestrel/Internal/Http/Frame.cs index d014f3d2b9..894b185dd5 100644 --- a/src/Microsoft.AspNetCore.Server.Kestrel/Internal/Http/Frame.cs +++ b/src/Microsoft.AspNetCore.Server.Kestrel/Internal/Http/Frame.cs @@ -339,13 +339,13 @@ namespace Microsoft.AspNetCore.Server.Kestrel.Internal.Http /// /// Immediate kill the connection and poison the request and response streams. /// - public void Abort() + public void Abort(Exception error = null) { if (Interlocked.CompareExchange(ref _requestAborted, 1, 0) == 0) { _requestProcessingStopping = true; - _frameStreams?.RequestBody.Abort(); + _frameStreams?.RequestBody.Abort(error); _frameStreams?.ResponseBody.Abort(); try diff --git a/src/Microsoft.AspNetCore.Server.Kestrel/Internal/Http/FrameRequestStream.cs b/src/Microsoft.AspNetCore.Server.Kestrel/Internal/Http/FrameRequestStream.cs index f2f7b379be..e9e295a593 100644 --- a/src/Microsoft.AspNetCore.Server.Kestrel/Internal/Http/FrameRequestStream.cs +++ b/src/Microsoft.AspNetCore.Server.Kestrel/Internal/Http/FrameRequestStream.cs @@ -13,6 +13,7 @@ namespace Microsoft.AspNetCore.Server.Kestrel.Internal.Http { private MessageBody _body; private FrameStreamState _state; + private Exception _error; public FrameRequestStream() { @@ -163,13 +164,15 @@ namespace Microsoft.AspNetCore.Server.Kestrel.Internal.Http _body = null; } - public void Abort() + public void Abort(Exception error = null) { // We don't want to throw an ODE until the app func actually completes. - // If the request is aborted, we throw an TaskCanceledException instead. + // If the request is aborted, we throw an TaskCanceledException instead, + // unless error is not null, in which case we throw it. if (_state != FrameStreamState.Closed) { _state = FrameStreamState.Aborted; + _error = error; } } @@ -186,7 +189,9 @@ namespace Microsoft.AspNetCore.Server.Kestrel.Internal.Http case FrameStreamState.Closed: throw new ObjectDisposedException(nameof(FrameRequestStream)); case FrameStreamState.Aborted: - return TaskUtilities.GetCancelledZeroTask(); + return _error != null ? + TaskUtilities.GetFaultedTask(_error) : + TaskUtilities.GetCancelledZeroTask(); } return null; } diff --git a/src/Microsoft.AspNetCore.Server.Kestrel/Internal/Http/SocketInput.cs b/src/Microsoft.AspNetCore.Server.Kestrel/Internal/Http/SocketInput.cs index dae06a5ce0..9c7f43031e 100644 --- a/src/Microsoft.AspNetCore.Server.Kestrel/Internal/Http/SocketInput.cs +++ b/src/Microsoft.AspNetCore.Server.Kestrel/Internal/Http/SocketInput.cs @@ -3,7 +3,6 @@ using System; using System.Diagnostics; -using System.IO; using System.Runtime.CompilerServices; using System.Threading; using System.Threading.Tasks; @@ -354,11 +353,7 @@ namespace Microsoft.AspNetCore.Server.Kestrel.Internal.Http var error = _tcs.Task.Exception?.InnerException; if (error != null) { - if (error is TaskCanceledException || error is InvalidOperationException) - { - throw error; - } - throw new IOException(error.Message, error); + throw error; } } } diff --git a/src/Microsoft.AspNetCore.Server.Kestrel/Internal/Infrastructure/TaskUtilities.cs b/src/Microsoft.AspNetCore.Server.Kestrel/Internal/Infrastructure/TaskUtilities.cs index 139411bca0..d5e828f0ee 100644 --- a/src/Microsoft.AspNetCore.Server.Kestrel/Internal/Infrastructure/TaskUtilities.cs +++ b/src/Microsoft.AspNetCore.Server.Kestrel/Internal/Infrastructure/TaskUtilities.cs @@ -1,6 +1,7 @@ // Copyright (c) .NET Foundation. All rights reserved. // Licensed under the Apache License, Version 2.0. See License.txt in the project root for license information. +using System; using System.Threading; using System.Threading.Tasks; @@ -39,6 +40,17 @@ namespace Microsoft.AspNetCore.Server.Kestrel.Internal.Infrastructure var tcs = new TaskCompletionSource(); tcs.TrySetCanceled(); return tcs.Task; +#endif + } + + public static Task GetFaultedTask(Exception error) + { +#if NETSTANDARD1_3 + return Task.FromException(error); +#else + var tcs = new TaskCompletionSource(); + tcs.SetException(error); + return tcs.Task; #endif } } diff --git a/test/Microsoft.AspNetCore.Server.Kestrel.FunctionalTests/RequestTests.cs b/test/Microsoft.AspNetCore.Server.Kestrel.FunctionalTests/RequestTests.cs index 2040f5de35..0d803c45da 100644 --- a/test/Microsoft.AspNetCore.Server.Kestrel.FunctionalTests/RequestTests.cs +++ b/test/Microsoft.AspNetCore.Server.Kestrel.FunctionalTests/RequestTests.cs @@ -248,25 +248,9 @@ namespace Microsoft.AspNetCore.Server.Kestrel.FunctionalTests { 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; + expectedExceptionThrown = ex.InnerException is UvException && ex.InnerException.Message.Contains("ECONNRESET"); } appDone.Release(); diff --git a/test/Microsoft.AspNetCore.Server.KestrelTests/FrameRequestStreamTests.cs b/test/Microsoft.AspNetCore.Server.KestrelTests/FrameRequestStreamTests.cs index 80368ee5d2..cbe4c4ccb6 100644 --- a/test/Microsoft.AspNetCore.Server.KestrelTests/FrameRequestStreamTests.cs +++ b/test/Microsoft.AspNetCore.Server.KestrelTests/FrameRequestStreamTests.cs @@ -114,5 +114,17 @@ namespace Microsoft.AspNetCore.Server.KestrelTests var task = stream.ReadAsync(new byte[1], 0, 1); Assert.True(task.IsCanceled); } + + [Fact] + public void AbortWithErrorCausesReadToCancel() + { + var stream = new FrameRequestStream(); + stream.StartAcceptingReads(null); + var error = new Exception(); + stream.Abort(error); + var task = stream.ReadAsync(new byte[1], 0, 1); + Assert.True(task.IsFaulted); + Assert.Same(error, task.Exception.InnerException); + } } } diff --git a/test/Microsoft.AspNetCore.Server.KestrelTests/TestHelpers/MockConnection.cs b/test/Microsoft.AspNetCore.Server.KestrelTests/TestHelpers/MockConnection.cs index efe612c47e..619d4d5b7b 100644 --- a/test/Microsoft.AspNetCore.Server.KestrelTests/TestHelpers/MockConnection.cs +++ b/test/Microsoft.AspNetCore.Server.KestrelTests/TestHelpers/MockConnection.cs @@ -17,7 +17,7 @@ namespace Microsoft.AspNetCore.Server.KestrelTests.TestHelpers RequestAbortedSource = new CancellationTokenSource(); } - public override void Abort() + public override void Abort(Exception error = null) { if (RequestAbortedSource != null) {