Added initial connection middleware pipeline #2003

davidfowl · 2017-08-18T03:03:52Z

Implemented IConnectionBuilder on ListenOptions. Kept IConnectionAdapter for now.
Delay the configure callback for ListenOptions until the server has started.
Added ConnectionLimitMiddleware and HttpConnectionMiddleware
Expose ConnectionAborted and ConnectionClosed on ConnectionContext and IConnectionTransportFeature
Updated the tests

Progress on #1980
Progress on #1772

- Implemented IConnectionBuilder on ListenOptions. Kept IConnectionAdapter for now. - Delay the configure callback for ListenOptions until the server has started. - Added ConnectionLimitMiddleware and HttpConnectionMiddleware - Expose ConnectionAborted and ConnectionClosed on ConnectionContext and IConnectionTransportFeature - Updated the tests

- Moved Application to IConnectionTransportFeature

halter73 · 2017-08-21T06:28:54Z

benchmarks/Kestrel.Performance/FrameWritingBenchmark.cs

-                Input = input.Reader,
-                Output = _outputPipe 
+                Application = pair.Application,
+                Transport = pair.Transport


I think you want to use _outputPipe here so the Cleanup method works.

Good catch.

halter73 · 2017-08-21T06:30:24Z

src/Protocols.Abstractions/Features/IConnectionTransportFeature.cs

+
+        Task ConnectionAborted { get; }
+
+        Task ConnectionClosed { get; }


Why can't a single Task represent aborted and closed?

Didn't we intend to communicate when/how the connection was closed via the IPipeConnections?

It's broken because of the dispatching. Ordering problems cause issues that I don't want to fix while changing everything else. This maps to the existing behavior.

One Task maps to the input being aborted (FIN)

One Task maps to when the output has closed

halter73 · 2017-08-21T06:32:56Z

src/Protocols.Abstractions/PipeFactoryExtensions.cs

+            var output = pipeFactory.Create(outputOptions);
+
+            var transportToApplication = new PipeConnection(output.Reader, input.Writer);
+            var applicationToTransport = new PipeConnection(input.Reader, output.Writer);


I like the TransportToApplication and ApplicationToTransport naming. Can we use this everywhere instead of of just Transport and Application?

No, it's too verbose. Also the only thing that usually sees both sides are when you're implementing the transport. Notice the DefaultConnectionContext still has the Transport exposed.

halter73 · 2017-08-21T06:36:15Z

src/Kestrel.Core/Internal/ConnectionHandler.cs

-            var inputPipe = transportFeature.PipeFactory.Create(GetInputPipeOptions(transportFeature.InputWriterScheduler));
-            var outputPipe = transportFeature.PipeFactory.Create(GetOutputPipeOptions(transportFeature.OutputReaderScheduler));
+            // REVIEW: Unfortunately, we still need to use the service context to create the pipes since the settings
+            // for the scheduler and limits are specified here


Could you just pass in the KestrelServerOptions? Do you need to pass in a scheduler and the "ThreadPool"?

halter73 · 2017-08-21T06:38:19Z

src/Kestrel.Core/Internal/ConnectionHandler.cs

-            // application code.
-            connection.StartRequestProcessing(_application);
+            // REVIEW: This task should be tracked by the server for graceful shutdown
+            // Today it's handled specifically for http but not for aribitrary middleware


Can we at least log a critical error if something goes wrong? I know that shouldn't/won't hapeen today, but this would also help avoid potential unobserved exceptions in the future.

halter73 · 2017-08-21T06:45:29Z

src/Kestrel.Core/ListenOptions.cs

@@ -126,6 +129,10 @@ public FileHandleType HandleType
        /// </remarks>
        public List<IConnectionAdapter> ConnectionAdapters { get; } = new List<IConnectionAdapter>();

+        public IServiceProvider ApplicationServices => KestrelServerOptions?.ApplicationServices;


Not again! Where is this used?

ApplicationServices is part of the interface. It will be used to activate middleware, just like IApplicationBuilder. ApplicationServices

halter73 · 2017-08-21T06:50:37Z

src/Kestrel.Core/KestrelServerOptions.cs

@@ -100,8 +100,7 @@ public void Listen(IPEndPoint endPoint, Action<ListenOptions> configure)
                throw new ArgumentNullException(nameof(configure));
            }

-            var listenOptions = new ListenOptions(endPoint) { KestrelServerOptions = this };
-            configure(listenOptions);
+            var listenOptions = new ListenOptions(endPoint) { KestrelServerOptions = this, Configure = configure };


Instead of adding this weird Configure callback, why not keep it the way it was and invoke configure directly in Listen()? Is this just for testing? Seems super confusing to me.

I need to run this later so that I can wrap the connection limit around the entire pipeline.

halter73 · 2017-08-21T06:54:30Z

src/Kestrel.Core/Internal/HttpConnectionMiddleware.cs

+
+namespace Microsoft.AspNetCore.Server.Kestrel.Core.Internal
+{
+    public class HttpConnectionMiddleware<TContext>


👍 I love that you were able to factor this out of ConnectionHandler.

halter73 · 2017-08-21T06:54:40Z

src/Kestrel.Core/Internal/HttpConnectionMiddleware.cs

+        }
+    }
+
+    public static class HttpConnectionBuilderExtensions


Move to a new file.

halter73 · 2017-08-21T06:56:19Z

src/Kestrel.Core/ListenOptions.cs

    {
        private FileHandleType _handleType;
+        private readonly List<Func<ConnectionDelegate, ConnectionDelegate>> _components = new List<Func<ConnectionDelegate, ConnectionDelegate>>();


halter73 · 2017-08-21T06:59:49Z

src/Kestrel.Core/Internal/ConnectionLimitMiddleware.cs

+        }
+    }
+
+    public static class ConnectionLimitBuilderExtensions


Move to new file.

davidfowl · 2017-08-21T07:50:19Z

🆙 📅

davidfowl requested review from analogrelay, halter73 and pakrym August 18, 2017 03:03

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

davidfowl added 3 commits August 19, 2017 00:25

Fixed benchmarks

aeb3efb

Removed IConnectionApplicationFeature

5c6786b

- Moved Application to IConnectionTransportFeature

davidfowl force-pushed the davidfowl/pipeline branch from 0262b43 to 5c6786b Compare August 19, 2017 07:36

Fixed the application pipe connection

52bca8a

halter73 suggested changes Aug 21, 2017

View reviewed changes

halter73 reviewed Aug 21, 2017

View reviewed changes

PR feedback

fa8d38b

halter73 approved these changes Aug 21, 2017

View reviewed changes

davidfowl merged commit 2e66870 into dev Aug 21, 2017

davidfowl deleted the davidfowl/pipeline branch November 9, 2017 08:11


		Task ConnectionAborted { get; }

		Task ConnectionClosed { get; }

Added initial connection middleware pipeline #2003

Added initial connection middleware pipeline #2003

Uh oh!

Conversation

davidfowl commented Aug 18, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

davidfowl Aug 21, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

davidfowl commented Aug 21, 2017

Uh oh!

Uh oh!

davidfowl commented Aug 18, 2017 •

edited

Loading

davidfowl Aug 21, 2017 •

edited

Loading