Move shutdown logic from transport to core #1707

halter73 · 2017-04-18T01:11:42Z

Use weak references to track FrameConnections which are equivalent to libuv references.

davidfowl · 2017-04-18T05:21:07Z

...Core.Server.Kestrel.Core/Internal/Infrastructure/FrameConnectionManagerShutdownExtensions.cs

+            });
+
+            var allAbortedTask = Task.WhenAll(abortTasks.ToArray());
+            return await Task.WhenAny(allAbortedTask, Task.Delay(1000)).ConfigureAwait(false) == allAbortedTask;


Hardcoding 1 second?

Abort was previously hard coded to one second. This timeout comes after the shutdown timeout is exceed/token canceled. What would you change this to?

davidfowl · 2017-04-18T05:54:53Z

src/Microsoft.AspNetCore.Server.Kestrel.Transport.Libuv/Internal/LibuvConnection.cs

@@ -86,29 +83,13 @@ public LibuvConnection()
                    // Ensure the socket is disposed prior to completing in the input writer.
                    _socket.Dispose();
                    Input.Complete(new TaskCanceledException("The request was aborted"));
-                    _socketClosedTcs.TrySetResult(null);
+                    _connectionContext.OnConnectionClosed();


The code makes it look like there's a chance that OnConnection can throw and as a result OnConnectionClosed won't be called.

I don't understand what you're getting at. What looks like it can throw?

davidfowl · 2017-04-18T06:19:50Z

test/Microsoft.AspNetCore.Server.Kestrel.FunctionalTests/MaxRequestBufferSizeTests.cs

@@ -165,7 +165,7 @@ public static IEnumerable<object[]> LargeUploadData

                    using (var reader = new StreamReader(stream, Encoding.ASCII))
                    {
-                        var response = reader.ReadToEnd();
+                        var response = await reader.ReadToEndAsync();


This needs to be synchronous for the timeout to work.

davidfowl · 2017-04-18T06:20:12Z

test/Microsoft.AspNetCore.Server.Kestrel.FunctionalTests/FrameConnectionManagerTests.cs

+        public async Task CriticalErrorLoggedIfApplicationDoesntComplete()
+        {
+            ////////////////////////////////////////////////////////////////////////////////////////
+            // WARNING: This test will fail under a debugger because Task.s_currentActiveTasks    //


😞. Should we let this test pass if Debugger.IsAttached?

davidfowl · 2017-04-18T06:21:49Z

src/Microsoft.AspNetCore.Server.Kestrel.Transport.Libuv/Internal/Networking/UvStreamHandle.cs

@@ -27,8 +27,6 @@ protected UvStreamHandle(ILibuvTrace logger) : base(logger)
        {
        }

-        public LibuvConnection Connection { get; set; }


pakrym · 2017-04-18T15:15:57Z

src/Microsoft.AspNetCore.Server.Kestrel.Core/Internal/Infrastructure/FrameConnectionManager.cs

    {
-        private readonly ConcurrentDictionary<long, FrameConnection> _connections
-            = new ConcurrentDictionary<long, FrameConnection>();
+        // Internal for testing


Undo internal

pakrym · 2017-04-18T15:18:06Z

test/Microsoft.AspNetCore.Server.Kestrel.FunctionalTests/FrameConnectionManagerTests.cs

+        public async Task CriticalErrorLoggedIfApplicationDoesntComplete()
+        {
+            ////////////////////////////////////////////////////////////////////////////////////////
+            // WARNING: This test will fail under a debugger because Task.s_currentActiveTasks    //


Use custom awaiter?

davidfowl · 2017-04-19T10:27:49Z

[xUnit.net 00:00:17.8537490]     ResponseStatusCodeSetBeforeHttpContextDisposeAppException [FAIL]
[xUnit.net 00:00:17.8561410]       System.AggregateException : One or more errors occurred. (Error -16 EBUSY resource busy or locked)
[xUnit.net 00:00:17.8562890]       ---- Microsoft.AspNetCore.Server.Kestrel.Transport.Libuv.Internal.Networking.UvException : Error -16 EBUSY resource busy or locked
[xUnit.net 00:00:17.8579300]       Stack Trace:
[xUnit.net 00:00:17.8597300]            at System.Threading.Tasks.Task.ThrowIfExceptional(Boolean includeTaskCanceledExceptions)
[xUnit.net 00:00:17.8598640]            at System.Threading.Tasks.Task.Wait(Int32 millisecondsTimeout, CancellationToken cancellationToken)
[xUnit.net 00:00:17.8599380]            at System.Threading.Tasks.Task.Wait()
[xUnit.net 00:00:17.8599880]            at Microsoft.AspNetCore.Server.Kestrel.FunctionalTests.TestServer.Dispose()
[xUnit.net 00:00:17.8600400]            at Microsoft.AspNetCore.Server.Kestrel.FunctionalTests.ResponseTests.<ResponseStatusCodeSetBeforeHttpContextDispose>d__12.MoveNext()
[xUnit.net 00:00:17.8600720]         --- End of stack trace from previous location where exception was thrown ---
[xUnit.net 00:00:17.8601330]            at System.Runtime.ExceptionServices.ExceptionDispatchInfo.Throw()
[xUnit.net 00:00:17.8601860]            at System.Runtime.CompilerServices.TaskAwaiter.HandleNonSuccessAndDebuggerNotification(Task task)
[xUnit.net 00:00:17.8602230]         --- End of stack trace from previous location where exception was thrown ---
[xUnit.net 00:00:17.8602710]            at System.Runtime.ExceptionServices.ExceptionDispatchInfo.Throw()
[xUnit.net 00:00:17.8603230]            at System.Runtime.CompilerServices.TaskAwaiter.HandleNonSuccessAndDebuggerNotification(Task task)
[xUnit.net 00:00:17.8603580]         --- End of stack trace from previous location where exception was thrown ---
[xUnit.net 00:00:17.8603970]            at System.Runtime.ExceptionServices.ExceptionDispatchInfo.Throw()
[xUnit.net 00:00:17.8604600]            at System.Runtime.CompilerServices.TaskAwaiter.HandleNonSuccessAndDebuggerNotification(Task task)
[xUnit.net 00:00:17.8605480]         ----- Inner Stack Trace -----
[xUnit.net 00:00:17.8606310]            at Microsoft.AspNetCore.Server.Kestrel.Transport.Libuv.Internal.Networking.LibuvFunctions.ThrowError(Int32 statusCode)
[xUnit.net 00:00:17.8606890]            at Microsoft.AspNetCore.Server.Kestrel.Transport.Libuv.Internal.Networking.LibuvFunctions.ThrowIfErrored(Int32 statusCode)
[xUnit.net 00:00:17.8607420]            at Microsoft.AspNetCore.Server.Kestrel.Transport.Libuv.Internal.Networking.LibuvFunctions.loop_close(UvLoopHandle handle)
[xUnit.net 00:00:17.8607900]            at Microsoft.AspNetCore.Server.Kestrel.Transport.Libuv.Internal.Networking.UvLoopHandle.ReleaseHandle()
[xUnit.net 00:00:17.8608330]            at System.Runtime.InteropServices.SafeHandle.InternalDispose()
[xUnit.net 00:00:17.8608780]            at Microsoft.AspNetCore.Server.Kestrel.Transport.Libuv.Internal.LibuvThread.ThreadStart(Object parameter)
[xUnit.net 00:00:17.8609120]         --- End of stack trace from previous location where exception was thrown ---
[xUnit.net 00:00:17.8609680]            at System.Runtime.ExceptionServices.ExceptionDispatchInfo.Throw()
[xUnit.net 00:00:17.8610180]            at Microsoft.AspNetCore.Server.Kestrel.Transport.Libuv.Internal.LibuvThread.<StopAsync>d__37.MoveNext()
[xUnit.net 00:00:17.8610580]         --- End of stack trace from previous location where exception was thrown ---
[xUnit.net 00:00:17.8611220]            at System.Runtime.ExceptionServices.ExceptionDispatchInfo.Throw()
[xUnit.net 00:00:17.8611680]            at System.Runtime.CompilerServices.TaskAwaiter.HandleNonSuccessAndDebuggerNotification(Task task)
[xUnit.net 00:00:17.8612160]            at Microsoft.AspNetCore.Server.Kestrel.Transport.Libuv.LibuvTransport.<StopAsync>d__19.MoveNext()
[xUnit.net 00:00:17.9084150]   Finished:    Microsoft.AspNetCore.Server.Kestrel.FunctionalTests
Failed   ResponseStatusCodeSetBeforeHttpContextDisposeAppException
Error Message:
 System.AggregateException : One or more errors occurred. (Error -16 EBUSY resource busy or locked)
---- Microsoft.AspNetCore.Server.Kestrel.Transport.Libuv.Internal.Networking.UvException : Error -16 EBUSY resource busy or locked
Stack Trace:

davidfowl · 2017-04-20T01:00:26Z

test/Microsoft.AspNetCore.Server.Kestrel.FunctionalTests/LoggingConnectionAdapterTests.cs

@@ -24,6 +24,7 @@ public class LoggingConnectionAdapterTests
                    {
                        listenOptions.UseConnectionLogging();
                        listenOptions.UseHttps(TestResources.TestCertificatePath, "testPassword");
+                        listenOptions.UseConnectionLogging();


What is this?

davidfowl

LGTM

dnfclas added the cla-already-signed label Apr 18, 2017

halter73 requested review from cesarblum, davidfowl and pakrym April 18, 2017 01:11

halter73 force-pushed the halter73/1585 branch from eeed474 to c6d8db5 Compare April 18, 2017 01:13

davidfowl reviewed Apr 18, 2017

View reviewed changes

pakrym reviewed Apr 18, 2017

View reviewed changes

halter73 force-pushed the halter73/1585 branch 3 times, most recently from 0155a10 to 3ffe334 Compare April 19, 2017 00:13

halter73 force-pushed the halter73/1585 branch 4 times, most recently from d10a05a to ec72961 Compare April 19, 2017 23:46

halter73 added 9 commits April 19, 2017 17:51

Move shutdown logic from transport to core

65556c2

Use weak references to track FrameConnections

0c77a62

Remove now unused Connection property from UvStreamHandle

f0fab12

PR feedback

949d311

More PR feedback

db3f128

Remove TimeoutTestServer

b9e2aa6

Fix TestServer

1158977

Complete FrameConnection.StopyAsync() after connection closed

fa85997

Fix rebase

64f0973

halter73 force-pushed the halter73/1585 branch from ff2b46e to 64f0973 Compare April 20, 2017 00:52

davidfowl reviewed Apr 20, 2017

View reviewed changes

davidfowl approved these changes Apr 20, 2017

View reviewed changes

halter73 merged commit a053ca4 into dev Apr 20, 2017

halter73 deleted the halter73/1585 branch April 20, 2017 01:08

natemcmaster mentioned this pull request Apr 21, 2017

NullReferenceException on Travis #1683

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Move shutdown logic from transport to core #1707

Move shutdown logic from transport to core #1707

halter73 commented Apr 18, 2017

davidfowl Apr 18, 2017

halter73 Apr 18, 2017

davidfowl Apr 18, 2017

halter73 Apr 18, 2017 •

edited

Loading

davidfowl Apr 18, 2017

davidfowl Apr 18, 2017 •

edited

Loading

davidfowl Apr 18, 2017

pakrym Apr 18, 2017

pakrym Apr 18, 2017

davidfowl commented Apr 19, 2017 •

edited

Loading

davidfowl Apr 20, 2017

davidfowl left a comment

Move shutdown logic from transport to core #1707

Move shutdown logic from transport to core #1707

Conversation

halter73 commented Apr 18, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

halter73 Apr 18, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

davidfowl Apr 18, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

davidfowl commented Apr 19, 2017 • edited Loading

Choose a reason for hiding this comment

davidfowl left a comment

Choose a reason for hiding this comment

halter73 Apr 18, 2017 •

edited

Loading

davidfowl Apr 18, 2017 •

edited

Loading

davidfowl commented Apr 19, 2017 •

edited

Loading