Create a direct way to configure endpoints on KestrelEngine #1280

halter73 · 2017-01-04T18:22:27Z

Replace endpoint configuration via .UseUrls() or --server.urls with Listen*
methods on KestrelSerrverOptions.
Replace IConnectionFilter with IConnectionAdapter which no longer exposes
ServerAddress via a context.
Simplify libuv Listener classes
Support systemd socket activation
Add docker-based test for systemd socket activation to be run on Travis

davidfowl · 2017-01-05T06:13:17Z

samples/SampleApp/Startup.cs

+                options.UseSystemd();
+
+                // The following section should be used to demo sockets
+                //options.ListenUnixSocket("http://unix:/tmp/kestrel-test.sock");


This should be:

options.ListenUnixSocket("/tmp/kestrel-test.sock");

davidfowl · 2017-01-05T06:13:55Z

src/Microsoft.AspNetCore.Server.Kestrel.Https/HttpsConnectionAdapter.cs

+            }
+
+            _options = options;
+            _logger = loggerFactory?.CreateLogger(nameof(HttpsConnectionAdapter));


When is the logger factory null?

When the HttpsConnectionAdapter is initialized directly using the other ctor.

davidfowl · 2017-01-05T06:16:35Z

src/Microsoft.AspNetCore.Server.Kestrel.Https/ListenOptionsHttpsExtensions.cs

+
+namespace Microsoft.AspNetCore.Hosting
+{
+    public static class ListenOptionsHttpsExtensions


nit: Add doc comment

davidfowl · 2017-01-05T09:53:25Z

src/Microsoft.AspNetCore.Server.Kestrel/Adapter/ListenOptionsConnectionLoggingExtensions.cs

+        /// <returns>
+        /// The <see cref="ListenOptions"/>.
+        /// </returns>
+        public static ListenOptions UseConnectionLogging(this ListenOptions listenOptions)


Why are these are listen options? Shouldn't it be on the top level kestrel options?

ConnectionAdapters are configured per endpoint. This is important for HTTPS.

If you want to configure all endpoints in the same way (e.g. to enable connection logging for everything), I would recommend creating a static method with a ListenOptions parameter to share the configuration code.

davidfowl · 2017-01-05T09:55:26Z

src/Microsoft.AspNetCore.Server.Kestrel/ListenOptions.cs

+        /// <remarks>
+        /// Defaults to empty.
+        /// </remarks>
+        public List<IConnectionAdapter> ConnectionAdapters { get; } = new List<IConnectionAdapter>();


Why have the filters moved to listen options? These should be ok KestrelServerOptions still no?

Different adapters per endpoint?

@CesarBS is right. Different adapters per endpoint is nice for HTTPS.

davidfowl · 2017-01-05T10:13:48Z

src/Microsoft.AspNetCore.Server.Kestrel/ListenOptions.cs

+        /// <remarks>
+        /// Defaults to true.
+        /// </remarks>
+        public bool NoDelay


We should consider splitting the configuration for accepted sockets vs the listen socket.

Even though we call uv_tcp_nodelay on the listen socket, I don't think it has any effect there (it might get inherited but that's pretty moot). I think the only important thing is configuring nodelay for the accepted sockets.

I certainly wouldn't split the configuration. We could remove the call uv_tcp_nodelay for the listen socket, but I would do that as part of another change and make absolutely sure that it doesn't affect perf.

davidfowl · 2017-01-05T10:17:32Z

src/Microsoft.AspNetCore.Server.Kestrel/Internal/Http/Listener.cs

+
+                    return pipe;
+                default:
+                    throw new InvalidOperationException();


NotSupportedException seems more appropriate

davidfowl · 2017-01-05T10:27:15Z

src/Microsoft.AspNetCore.Server.Kestrel/Internal/Http/Connection.cs


-            _frame.PrepareRequest = _filterContext.PrepareRequest;
-            _frame.Start();
+                _frame.PrepareRequest = features =>


You could avoid the allocation by setting the list of adaptedConnections on the frame itself and having it do this logic in Frame. It's actually a bit cleaner than assigning a delegate in Connection.

davidfowl · 2017-01-05T10:29:05Z

src/Microsoft.AspNetCore.Server.Kestrel/Internal/Http/PipeListener.cs

@@ -1,61 +0,0 @@
-// Copyright (c) .NET Foundation. All rights reserved.


These deleted files make me so happy 👍

davidfowl · 2017-01-05T10:29:37Z

src/Microsoft.AspNetCore.Server.Kestrel/Internal/Networking/SockAddr.cs

@@ -92,7 +92,7 @@ public unsafe IPEndPoint GetIPEndPoint()
                    *((long*)(b + 8)) = _field2;
                }

-                return new IPEndPoint(new IPAddress(bytes), port);
+                return new IPEndPoint(new IPAddress(bytes, scopeid: _field3 & 0xFFFFFFFF), port);


This is comment worthy.

I added to the big comment above.

cesarblum · 2017-01-05T17:44:35Z

src/Microsoft.AspNetCore.Server.Kestrel/Internal/Http/Connection.cs

-        private LibuvStream _libuvStream;
-        private FilteredStreamAdapter _filteredStreamAdapter;
+        private readonly List<IConnectionAdapter> _connectionAdapters;
+        private AdaptedPipeline _filteredStreamAdapter;


nit: rename to _adaptedPipeline?

cesarblum · 2017-01-05T17:48:28Z

src/Microsoft.AspNetCore.Server.Kestrel/Internal/Http/Connection.cs

+                    }
+                    catch (Exception ex)
+                    {
+                        Log.LogError(0, ex, "ConnectioAdapter.PrepareRequest");


ConnectioAdapter -> ConnectionAdapter

use nameof 😁

cesarblum · 2017-01-05T17:48:40Z

src/Microsoft.AspNetCore.Server.Kestrel/Internal/Http/Connection.cs

+            }
+            catch (Exception ex)
+            {
+                Log.LogError(0, ex, "ConnectioAdapter.OnConnectionAsync");


same typo here

cesarblum · 2017-01-05T17:50:35Z

src/Microsoft.AspNetCore.Server.Kestrel/Internal/Http/Listener.cs

+            }
+            catch (UvException ex)
+            {
+                Log.LogError(0, ex, "Listener.OnConnection");


use nameof

Wouldn't this need to be the following then?

Log.LogError(0, ex, $"{nameof(Listener)}.{nameof(Listener.OnConnection)}");

Not sure it's worth it since we never really expect the above to throw. If you search the Kestrel code base for "Log.LogError" you'll see a lot of places that need to change.

I tend to prefer nameof for things like this because it's easy to miss that if there's a rename.

cesarblum · 2017-01-05T17:53:38Z

src/Microsoft.AspNetCore.Server.Kestrel/Internal/KestrelEngine.cs

@@ -61,49 +61,33 @@ public void Dispose()
 #endif
        }

-        public IDisposable CreateServer(ServerAddress address)
+        public IDisposable CreateServer(ListenOptions listenOptions)


Soooo much better to read 👍

cesarblum · 2017-01-05T18:03:21Z

src/Microsoft.AspNetCore.Server.Kestrel/ListenOptions.cs

+        /// <remarks>
+        /// Defaults to empty.
+        /// </remarks>
+        public List<IConnectionAdapter> ConnectionAdapters { get; } = new List<IConnectionAdapter>();


Different adapters per endpoint?

cesarblum · 2017-01-05T18:06:00Z

src/Microsoft.AspNetCore.Server.Kestrel/Systemd/KesterlServerOptionsSystemdExtensions.cs

+
+namespace Microsoft.AspNetCore.Hosting
+{
+    public static class KesterlServerOptionsSystemdExtensions


We should offer this in a separate package.

cesarblum · 2017-01-05T18:10:37Z

test/Microsoft.AspNetCore.Server.Kestrel.FunctionalTests/SystemdActivation/docker.sh

@@ -0,0 +1,19 @@
+#!/usr/bin/env bash


Add set -e at the top to fail the script if any command fails.

cesarblum · 2017-01-05T18:14:51Z

test/Microsoft.AspNetCore.Server.Kestrel.FunctionalTests/SystemdActivation/docker.sh

+
+# Ensure that dotnet is added to the PATH
+scriptDir=$(dirname "${BASH_SOURCE[0]}")
+repoDir=$(cd $scriptDir/../../.. && pwd)


You can replace cd $scriptDir/../../.. && pwd with readlink -e $scriptDir/../../..

I actually want to change directories.

cesarblum · 2017-01-05T18:16:09Z

test/Microsoft.AspNetCore.Server.KestrelTests/KestrelServerOptionsTests.cs

+        }
+
+        [Fact]
+        public void NoDelayPropogatesToListenOptions()


nit: Propagates

Tratcher

Here's the WebListener API for reference:
https://github.com/aspnet/HttpSysServer/blob/dev/src/Microsoft.AspNetCore.Server.HttpSys/UrlPrefixCollection.cs#L38-L43
https://github.com/aspnet/HttpSysServer/blob/dev/src/Microsoft.AspNetCore.Server.HttpSys/UrlPrefix.cs

It's a lot simpler because there are no per-endpoint settings. All the information is captured in the prefix or stored via netsh.

Tratcher · 2017-01-06T04:27:56Z

samples/SampleApp/Startup.cs

-                .UseKestrel(options =>
+            var hostBuilder = new WebHostBuilder().UseKestrel(options =>
+            {
+                options.Listen(IPAddress.Loopback, 5000, listenOptions =>


For a realistic sample the IP and port need to come from command line / env / config. We need to make that possible in one or two lines. Even if it means having a Listen overload that takes a string for IP:port that understands IPv4, IPv6, and *.
@shirhatti
options.Listen(config["IPAndPort"], listenOptions => ...

Note IPEndpoint does not implement Parse so you can't just punt this over there.

Note that the VS 2017 self-host profile now sets the ASPNETCORE_URLS environment variable by default so apps are unlikely to start from VS on localhost:5000 anymore. /cc: @BillHiebert

Tratcher · 2017-01-06T04:38:34Z

src/Microsoft.AspNetCore.Server.Kestrel/KestrelServer.cs

+                if (listenOptions.Any())
+                {
+                    var addresses = _serverAddresses.Addresses;
+                    if (addresses.SingleOrDefault() != "http://localhost:5000")


weird guesswork that makes assumptions about the intent of the caller. I'd rather reconsider the Hosting default logic. WebListener is going to have a similar issue when it's configured to attach to an existing queue. @davidfowl

AFIAK, the idea is to remove the IServerAddresses feature entirely prior to 2.0, so this heuristic is only temporary.

Remove IServerAddresses? What's the rest of that plan?

Tratcher · 2017-01-06T04:46:58Z

src/Microsoft.AspNetCore.Server.Kestrel/Systemd/KesterlServerOptionsSystemdExtensions.cs

+            if (string.Equals(Process.GetCurrentProcess().Id.ToString(CultureInfo.InvariantCulture), Environment.GetEnvironmentVariable("LISTEN_PID"), StringComparison.Ordinal))
+            {
+                // SD_LISTEN_FDS_START = 3
+                options.ListenHandle(3, configure);


define constants

cesarblum

One minor follow-up comment but LGTM otherwise.

- Replace endpoint configuration via .UseUrls() or --server.urls with Listen* methods on KestrelSerrverOptions. - Replace IConnectionFilter with IConnectionAdapter which no longer exposes ServerAddress via a context. - Simplify libuv Listener classes - Support systemd socket activation - Add docker-based test for systemd socket activation to be run on Travis

dnfclas added the cla-already-signed label Jan 4, 2017

halter73 force-pushed the halter73/996 branch 4 times, most recently from 51ef5c5 to 9e2947f Compare January 5, 2017 03:08

davidfowl suggested changes Jan 5, 2017

View reviewed changes

cesarblum reviewed Jan 5, 2017

View reviewed changes

Tratcher reviewed Jan 6, 2017

View reviewed changes

cesarblum approved these changes Jan 6, 2017

View reviewed changes

halter73 force-pushed the halter73/996 branch from 7ed030a to 2351c1b Compare January 7, 2017 01:38

halter73 merged commit 2351c1b into dev Jan 7, 2017

halter73 deleted the halter73/996 branch January 7, 2017 01:38

halter73 mentioned this pull request Jan 7, 2017

Support systemd socket activation #1057

Closed

Tratcher mentioned this pull request Jan 7, 2017

Make it easier to configure Kestrel endpoints from config #1290

Closed

natemcmaster mentioned this pull request Aug 15, 2017

Add fixes for problems discovered by xunit.analyzers #1999

Merged

		@@ -1,61 +0,0 @@
		// Copyright (c) .NET Foundation. All rights reserved.

Create a direct way to configure endpoints on KestrelEngine #1280

Create a direct way to configure endpoints on KestrelEngine #1280

Uh oh!

Conversation

halter73 commented Jan 4, 2017

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!

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!

Tratcher left a comment

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