PR feedback

aspnet · Apr 18, 2017 · 0155a10 · 0155a10
1 parent 2dc7295
commit 0155a10
Show file tree

Hide file tree

Showing 7 changed files with 24 additions and 19 deletions.
diff --git a/src/Microsoft.AspNetCore.Server.Kestrel.Core/Internal/FrameConnection.cs b/src/Microsoft.AspNetCore.Server.Kestrel.Core/Internal/FrameConnection.cs
@@ -12,7 +12,6 @@
 using Microsoft.AspNetCore.Server.Kestrel.Core.Internal.Infrastructure;
 using Microsoft.AspNetCore.Server.Kestrel.Internal.System.IO.Pipelines;
 using Microsoft.AspNetCore.Server.Kestrel.Transport.Abstractions;
-using Microsoft.Extensions.Internal;
 using Microsoft.Extensions.Logging;
 
 namespace Microsoft.AspNetCore.Server.Kestrel.Core.Internal
@@ -22,15 +21,15 @@ public class FrameConnection : IConnectionContext, ITimeoutControl
         private readonly FrameConnectionContext _context;
         private readonly Frame _frame;
         private readonly List<IConnectionAdapter> _connectionAdapters;
-        private readonly TaskCompletionSource<object> _frameStartedTcs = new TaskCompletionSource<object>();
+        private readonly TaskCompletionSource<bool> _frameStartedTcs = new TaskCompletionSource<bool>();
 
         private long _lastTimestamp;
         private long _timeoutTimestamp = long.MaxValue;
         private TimeoutAction _timeoutAction;
 
         private AdaptedPipeline _adaptedPipeline;
         private Stream _filteredStream;
-        private Task _adaptedPipelineTask = TaskCache.CompletedTask;
+        private Task _adaptedPipelineTask;
 
         public FrameConnection(FrameConnectionContext context)
         {
@@ -101,9 +100,11 @@ public async void OnConnectionClosed()
 
         public async Task StopAsync()
         {
-            await _frameStartedTcs.Task;
-            await _frame.StopAsync();
-            await _adaptedPipelineTask;
+            if (await _frameStartedTcs.Task)
+            {
+                await _frame.StopAsync();
+                await (_adaptedPipelineTask ?? Task.CompletedTask);
+            }
         }
 
         public void Abort(Exception ex)
@@ -158,7 +159,7 @@ private async Task ApplyConnectionAdaptersAsync()
             catch (Exception ex)
             {
                 Log.LogError(0, ex, $"Uncaught exception from the {nameof(IConnectionAdapter.OnConnectionAsync)} method of an {nameof(IConnectionAdapter)}.");
-                _frameStartedTcs.SetResult(null);
+                _frameStartedTcs.SetResult(false);
                 CloseRawPipes();
             }
         }
@@ -191,7 +192,7 @@ private void StartFrame()
         {
             _lastTimestamp = _context.ServiceContext.SystemClock.UtcNow.Ticks;
             _frame.Start();
-            _frameStartedTcs.SetResult(null);
+            _frameStartedTcs.SetResult(true);
         }
 
         public void Tick(DateTimeOffset now)

diff --git a/src/Microsoft.AspNetCore.Server.Kestrel.Core/Internal/Http/Frame.cs b/src/Microsoft.AspNetCore.Server.Kestrel.Core/Internal/Http/Frame.cs
@@ -430,7 +430,7 @@ public Task StopAsync()
             Input.CancelPendingRead();
 
             Debug.Assert(_requestProcessingTask != null);
-            return _requestProcessingTask;
+            return _requestProcessingTask ?? Task.CompletedTask;
         }
 
         private void CancelRequestAbortedToken()

diff --git a/...icrosoft.AspNetCore.Server.Kestrel.Core/Internal/Infrastructure/FrameConnectionManager.cs b/...icrosoft.AspNetCore.Server.Kestrel.Core/Internal/Infrastructure/FrameConnectionManager.cs
@@ -8,8 +8,7 @@ namespace Microsoft.AspNetCore.Server.Kestrel.Core.Internal.Infrastructure
 {
     public class FrameConnectionManager
     {
-        // Internal for testing
-        internal readonly ConcurrentDictionary<long, FrameConnectionReference> _connectionReferences = new ConcurrentDictionary<long, FrameConnectionReference>();
+        private readonly ConcurrentDictionary<long, FrameConnectionReference> _connectionReferences = new ConcurrentDictionary<long, FrameConnectionReference>();
         private readonly IKestrelTrace _trace;
 
         public FrameConnectionManager(IKestrelTrace trace)

diff --git a/src/Microsoft.AspNetCore.Server.Kestrel.Transport.Libuv/Internal/LibuvThread.cs b/src/Microsoft.AspNetCore.Server.Kestrel.Transport.Libuv/Internal/LibuvThread.cs
@@ -239,9 +239,6 @@ private void ThreadStart(object parameter)
                 }
             }
 
-            // This is used to access a 64-bit timestamp (this.Now) using a potentially 32-bit IntPtr.
-            var thisHandle = GCHandle.Alloc(this, GCHandleType.Weak);
-
             try
             {
                 _loop.Run();
@@ -271,7 +268,6 @@ private void ThreadStart(object parameter)
             {
                 PipeFactory.Dispose();
                 WriteReqPool.Dispose();
-                thisHandle.Free();
                 _threadTcs.SetResult(null);
             }
         }

diff --git a/test/Microsoft.AspNetCore.Server.Kestrel.Core.Tests/FrameTests.cs b/test/Microsoft.AspNetCore.Server.Kestrel.Core.Tests/FrameTests.cs
@@ -446,12 +446,12 @@ public void RequestProcessingAsyncEnablesKeepAliveTimeout()
             var connectionControl = new Mock<ITimeoutControl>();
             _frame.TimeoutControl = connectionControl.Object;
 
-            var requestProcessingTask = _frame.RequestProcessingAsync();
+            _frame.Start();
 
             var expectedKeepAliveTimeout = _serviceContext.ServerOptions.Limits.KeepAliveTimeout.Ticks;
             connectionControl.Verify(cc => cc.SetTimeout(expectedKeepAliveTimeout, TimeoutAction.CloseConnection));
 
-            _frame.StopAsync();
+            var requestProcessingTask = _frame.StopAsync();
             _input.Writer.Complete();
 
             requestProcessingTask.Wait();

diff --git a/test/Microsoft.AspNetCore.Server.Kestrel.FunctionalTests/FrameConnectionManagerTests.cs b/test/Microsoft.AspNetCore.Server.Kestrel.FunctionalTests/FrameConnectionManagerTests.cs
@@ -2,11 +2,13 @@
 // Licensed under the Apache License, Version 2.0. See License.txt in the project root for license information.
 
 using System;
+using System.Diagnostics;
 using System.Threading;
 using System.Threading.Tasks;
 using Microsoft.AspNetCore.Builder;
 using Microsoft.AspNetCore.Hosting;
 using Microsoft.AspNetCore.Testing;
+using Microsoft.AspNetCore.Testing.xunit;
 using Microsoft.Extensions.Logging;
 using Moq;
 using Xunit;
@@ -17,7 +19,8 @@ public class FrameConnectionManagerTests
     {
         private const int _applicationNeverCompletedId = 23;
 
-        [Fact]
+        [ConditionalFact]
+        [NoDebuggerCondition]
         public async Task CriticalErrorLoggedIfApplicationDoesntComplete()
         {
             ////////////////////////////////////////////////////////////////////////////////////////
@@ -89,5 +92,11 @@ await connection.Send("GET / HTTP/1.1",
                 Assert.True(logWaitAttempts < 10);
             }
         }
+
+        private class NoDebuggerConditionAttribute : Attribute, ITestCondition
+        {
+            public bool IsMet => !Debugger.IsAttached;
+            public string SkipReason => "A debugger is attached.";
+        }
     }
 }
diff --git a/test/Microsoft.AspNetCore.Server.Kestrel.FunctionalTests/MaxRequestBufferSizeTests.cs b/test/Microsoft.AspNetCore.Server.Kestrel.FunctionalTests/MaxRequestBufferSizeTests.cs
@@ -165,7 +165,7 @@ public async Task LargeUpload(long? maxRequestBufferSize, bool connectionAdapter
 
                     using (var reader = new StreamReader(stream, Encoding.ASCII))
                     {
-                        var response = await reader.ReadToEndAsync();
+                        var response = reader.ReadToEnd();
                         Assert.Contains($"bytesRead: {data.Length}", response);
                     }
                 }