Handle client and server aborts differently from eachhother (#2612)

* Trace when app aborts connection * Improve exception messages * Always abort connection with ConnectionAbortedException * Add ConnectionContext.Abort(Exception)
aspnet · Jun 1, 2018 · 0aff4a0 · 0aff4a0
1 parent aa9b9ca
commit 0aff4a0
Show file tree

Hide file tree

Showing 36 changed files with 300 additions and 143 deletions.
diff --git a/benchmarks/Kestrel.Performance/InMemoryTransportBenchmark.cs b/benchmarks/Kestrel.Performance/InMemoryTransportBenchmark.cs
@@ -3,6 +3,7 @@
 
 using BenchmarkDotNet.Attributes;
 using Microsoft.AspNetCore.Builder;
+using Microsoft.AspNetCore.Connections;
 using Microsoft.AspNetCore.Hosting;
 using Microsoft.AspNetCore.Http;
 using Microsoft.AspNetCore.Server.Kestrel.Transport.Abstractions.Internal;
@@ -204,6 +205,10 @@ public async Task<byte[]> GetResponseAsync(int length)
                     }
                 }
             }
+
+            protected override void AbortCore(ConnectionAbortedException abortReason)
+            {
+            }
         }
 
         // Copied from https://github.com/aspnet/benchmarks/blob/dev/src/Benchmarks/Middleware/PlaintextMiddleware.cs

diff --git a/benchmarks/Kestrel.Performance/Mocks/MockTrace.cs b/benchmarks/Kestrel.Performance/Mocks/MockTrace.cs
@@ -44,6 +44,7 @@ public class MockTrace : IKestrelTrace
         public void RequestBodyDrainTimedOut(string connectionId, string traceIdentifier) { }
         public void RequestBodyMininumDataRateNotSatisfied(string connectionId, string traceIdentifier, double rate) { }
         public void ResponseMininumDataRateNotSatisfied(string connectionId, string traceIdentifier) { }
+        public void ApplicationAbortedConnection(string connectionId, string traceIdentifier) { }
         public void Http2ConnectionError(string connectionId, Http2ConnectionErrorException ex) { }
         public void Http2StreamError(string connectionId, Http2StreamErrorException ex) { }
         public void HPackDecodingError(string connectionId, int streamId, HPackDecodingException ex) { }

diff --git a/src/Connections.Abstractions/ConnectionContext.cs b/src/Connections.Abstractions/ConnectionContext.cs
@@ -1,5 +1,9 @@
-using System.Collections.Generic;
+// 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.Collections.Generic;
 using System.IO.Pipelines;
+using Microsoft.AspNetCore.Connections.Features;
 using Microsoft.AspNetCore.Http.Features;
 
 namespace Microsoft.AspNetCore.Connections
@@ -13,5 +17,13 @@ public abstract class ConnectionContext
         public abstract IDictionary<object, object> Items { get; set; }
 
         public abstract IDuplexPipe Transport { get; set; }
+
+        public virtual void Abort(ConnectionAbortedException abortReason)
+        {
+            // We expect this to be overridden, but this helps maintain back compat
+            // with implementations of ConnectionContext that predate the addition of
+            // ConnectioContext.Abort()
+            Features.Get<IConnectionLifetimeFeature>()?.Abort();
+        }
     }
 }
diff --git a/src/Connections.Abstractions/DefaultConnectionContext.cs b/src/Connections.Abstractions/DefaultConnectionContext.cs
@@ -65,7 +65,9 @@ public DefaultConnectionContext(string id, IDuplexPipe transport, IDuplexPipe ap
 
         public CancellationToken ConnectionClosed { get; set; }
 
-        public virtual void Abort()
+        public void Abort() => Abort(abortReason: null);
+
+        public override void Abort(ConnectionAbortedException abortReason)
         {
             ThreadPool.QueueUserWorkItem(cts => ((CancellationTokenSource)cts).Cancel(), _connectionClosedTokenSource);
         }

diff --git a/src/Connections.Abstractions/Features/IConnectionLifetimeFeature.cs b/src/Connections.Abstractions/Features/IConnectionLifetimeFeature.cs
@@ -8,6 +8,7 @@ namespace Microsoft.AspNetCore.Connections.Features
     public interface IConnectionLifetimeFeature
     {
         CancellationToken ConnectionClosed { get; set; }
+
         void Abort();
     }
 }
diff --git a/src/Kestrel.Core/Adapter/Internal/AdaptedPipeline.cs b/src/Kestrel.Core/Adapter/Internal/AdaptedPipeline.cs
@@ -6,7 +6,9 @@
 using System.IO.Pipelines;
 using System.Threading.Tasks;
 using Microsoft.AspNetCore.Server.Kestrel.Core.Internal.Http;
+using Microsoft.AspNetCore.Server.Kestrel.Core.Internal.Infrastructure;
 using Microsoft.AspNetCore.Server.Kestrel.Transport.Abstractions.Internal;
+using Microsoft.Extensions.Logging;
 
 namespace Microsoft.AspNetCore.Server.Kestrel.Core.Adapter.Internal
 {
@@ -15,23 +17,24 @@ public class AdaptedPipeline : IDuplexPipe
         private static readonly int MinAllocBufferSize = KestrelMemoryPool.MinimumSegmentSize / 2;
 
         private readonly IDuplexPipe _transport;
-        private readonly IDuplexPipe _application;
 
         public AdaptedPipeline(IDuplexPipe transport,
-                               IDuplexPipe application,
                                Pipe inputPipe,
-                               Pipe outputPipe)
+                               Pipe outputPipe,
+                               IKestrelTrace log)
         {
             _transport = transport;
-            _application = application;
             Input = inputPipe;
             Output = outputPipe;
+            Log = log;
         }
 
         public Pipe Input { get; }
 
         public Pipe Output { get; }
 
+        public IKestrelTrace Log { get; }
+
         PipeReader IDuplexPipe.Input => Input.Reader;
 
         PipeWriter IDuplexPipe.Output => Output.Writer;
@@ -47,8 +50,6 @@ public async Task RunAsync(Stream stream)
 
         private async Task WriteOutputAsync(Stream stream)
         {
-            Exception error = null;
-
             try
             {
                 if (stream == null)
@@ -63,13 +64,6 @@ private async Task WriteOutputAsync(Stream stream)
 
                     try
                     {
-                        if (result.IsCanceled)
-                        {
-                            // Forward the cancellation to the transport pipe
-                            _application.Input.CancelPendingRead();
-                            break;
-                        }
-
                         if (buffer.IsEmpty)
                         {
                             if (result.IsCompleted)
@@ -108,7 +102,7 @@ private async Task WriteOutputAsync(Stream stream)
             }
             catch (Exception ex)
             {
-                error = ex;
+                Log.LogError(0, ex, $"{nameof(AdaptedPipeline)}.{nameof(WriteOutputAsync)}");
             }
             finally
             {

diff --git a/src/Kestrel.Core/CoreStrings.resx b/src/Kestrel.Core/CoreStrings.resx
@@ -506,4 +506,16 @@ For more information on configuring HTTPS see https://go.microsoft.com/fwlink/?l
   <data name="BadRequest_RequestBodyTimeout" xml:space="preserve">
     <value>Reading the request body timed out due to data arriving too slowly. See MinRequestBodyDataRate.</value>
   </data>
+  <data name="ConnectionAbortedByApplication" xml:space="preserve">
+    <value>The connection was aborted by the application.</value>
+  </data>
+  <data name="ConnectionAbortedDuringServerShutdown" xml:space="preserve">
+    <value>The connection was aborted because the server is shutting down and request processing didn't complete within the time specified by HostOptions.ShutdownTimeout.</value>
+  </data>
+  <data name="ConnectionTimedBecauseResponseMininumDataRateNotSatisfied" xml:space="preserve">
+    <value>The connection was timed out by the server because the response was not read by the client at the specified minimum data rate.</value>
+  </data>
+  <data name="ConnectionTimedOutByServer" xml:space="preserve">
+    <value>The connection was timed out by the server.</value>
+  </data>
 </root>
diff --git a/src/Kestrel.Core/Internal/Http/Http1Connection.cs b/src/Kestrel.Core/Internal/Http/Http1Connection.cs
@@ -45,12 +45,11 @@ public Http1Connection(Http1ConnectionContext context)
             _requestHeadersTimeoutTicks = ServerOptions.Limits.RequestHeadersTimeout.Ticks;
 
             Output = new Http1OutputProducer(
-                _context.Application.Input,
                 _context.Transport.Output,
                 _context.ConnectionId,
+                _context.ConnectionContext,
                 _context.ServiceContext.Log,
                 _context.TimeoutControl,
-                _context.ConnectionFeatures.Get<IConnectionLifetimeFeature>(),
                 _context.ConnectionFeatures.Get<IBytesWrittenFeature>());
         }
 

diff --git a/src/Kestrel.Core/Internal/Http/Http1ConnectionContext.cs b/src/Kestrel.Core/Internal/Http/Http1ConnectionContext.cs
@@ -4,6 +4,7 @@
 using System.Buffers;
 using System.IO.Pipelines;
 using System.Net;
+using Microsoft.AspNetCore.Connections;
 using Microsoft.AspNetCore.Http.Features;
 using Microsoft.AspNetCore.Server.Kestrel.Core.Internal.Infrastructure;
 
@@ -13,6 +14,7 @@ public class Http1ConnectionContext : IHttpProtocolContext
     {
         public string ConnectionId { get; set; }
         public ServiceContext ServiceContext { get; set; }
+        public ConnectionContext ConnectionContext { get; set; }
         public IFeatureCollection ConnectionFeatures { get; set; }
         public MemoryPool<byte> MemoryPool { get; set; }
         public IPEndPoint RemoteEndPoint { get; set; }

diff --git a/src/Kestrel.Core/Internal/Http/Http1OutputProducer.cs b/src/Kestrel.Core/Internal/Http/Http1OutputProducer.cs
@@ -8,6 +8,7 @@
 using System.Text;
 using System.Threading;
 using System.Threading.Tasks;
+using Microsoft.AspNetCore.Connections;
 using Microsoft.AspNetCore.Connections.Features;
 using Microsoft.AspNetCore.Server.Kestrel.Core.Internal.Infrastructure;
 using Microsoft.AspNetCore.Server.Kestrel.Transport.Abstractions.Internal;
@@ -22,9 +23,9 @@ public class Http1OutputProducer : IHttpOutputProducer
         private static readonly ReadOnlyMemory<byte> _endChunkedResponseBytes = new ReadOnlyMemory<byte>(Encoding.ASCII.GetBytes("0\r\n\r\n"));
 
         private readonly string _connectionId;
+        private readonly ConnectionContext _connectionContext;
         private readonly ITimeoutControl _timeoutControl;
         private readonly IKestrelTrace _log;
-        private readonly IConnectionLifetimeFeature _lifetimeFeature;
         private readonly IBytesWrittenFeature _transportBytesWrittenFeature;
 
         // This locks access to to all of the below fields
@@ -36,7 +37,6 @@ public class Http1OutputProducer : IHttpOutputProducer
         private long _totalBytesCommitted;
 
         private readonly PipeWriter _pipeWriter;
-        private readonly PipeReader _outputPipeReader;
 
         // https://github.com/dotnet/corefxlab/issues/1334
         // Pipelines don't support multiple awaiters on flush
@@ -48,21 +48,19 @@ public class Http1OutputProducer : IHttpOutputProducer
         private ValueTask<FlushResult> _flushTask;
 
         public Http1OutputProducer(
-            PipeReader outputPipeReader,
             PipeWriter pipeWriter,
             string connectionId,
+            ConnectionContext connectionContext,
             IKestrelTrace log,
             ITimeoutControl timeoutControl,
-            IConnectionLifetimeFeature lifetimeFeature,
             IBytesWrittenFeature transportBytesWrittenFeature)
         {
-            _outputPipeReader = outputPipeReader;
             _pipeWriter = pipeWriter;
             _connectionId = connectionId;
+            _connectionContext = connectionContext;
             _timeoutControl = timeoutControl;
             _log = log;
             _flushCompleted = OnFlushCompleted;
-            _lifetimeFeature = lifetimeFeature;
             _transportBytesWrittenFeature = transportBytesWrittenFeature;
         }
 
@@ -169,7 +167,7 @@ public void Dispose()
             }
         }
 
-        public void Abort(Exception error)
+        public void Abort(ConnectionAbortedException error)
         {
             // Abort can be called after Dispose if there's a flush timeout.
             // It's important to still call _lifetimeFeature.Abort() in this case.
@@ -181,17 +179,15 @@ public void Abort(Exception error)
                     return;
                 }
 
+                _aborted = true;
+                _connectionContext.Abort(error);
+
                 if (!_completed)
                 {
                     _log.ConnectionDisconnect(_connectionId);
                     _completed = true;
-
-                    _outputPipeReader.CancelPendingRead();
-                    _pipeWriter.Complete(error);
+                    _pipeWriter.Complete();
                 }
-
-                _aborted = true;
-                _lifetimeFeature.Abort();
             }
         }
 

diff --git a/src/Kestrel.Core/Internal/Http/HttpProtocol.FeatureCollection.cs b/src/Kestrel.Core/Internal/Http/HttpProtocol.FeatureCollection.cs
@@ -227,7 +227,8 @@ void IHttpResponseFeature.OnCompleted(Func<object, Task> callback, object state)
 
         void IHttpRequestLifetimeFeature.Abort()
         {
-            Abort(new ConnectionAbortedException());
+            Log.ApplicationAbortedConnection(ConnectionId, TraceIdentifier);
+            Abort(new ConnectionAbortedException(CoreStrings.ConnectionAbortedByApplication));
         }
     }
 }