Remove connection adapters and move things to middleware #11412

davidfowl · 2019-06-20T09:27:26Z

Remove connection adapters from the public API surface (pubternal) and replace the existing adapters with connection middleware.
Updated the tests

- Remove connection adapters from the public API surface (pubternal) and replace the existing adapters with connection middleware. - Updated the tests

davidfowl · 2019-06-20T09:55:31Z

src/Servers/Kestrel/Core/ref/Microsoft.AspNetCore.Server.Kestrel.Core.netcoreapp3.0.cs

@@ -160,24 +159,6 @@ public partial class MinDataRate
        public System.TimeSpan GracePeriod { [System.Runtime.CompilerServices.CompilerGeneratedAttribute]get { throw null; } }
    }
 }
-namespace Microsoft.AspNetCore.Server.Kestrel.Core.Adapter.Internal


@sebastienros This will break the benchmarks.

Specifically this https://github.com/aspnet/Benchmarks/blob/fd69bc294bdfda8d23a9e094dfbbb4a56fa82651/src/Benchmarks/PassthroughConnectionFilter.cs#L12

davidfowl · 2019-06-20T10:00:28Z

src/Servers/Kestrel/Core/src/Internal/HttpsConnectionMiddleware.cs

@@ -281,5 +260,19 @@ private static X509Certificate2 ConvertToX509Certificate2(X509Certificate certif

            return new X509Certificate2(certificate);
        }
+
+        private class SslDuplexPipe : DuplexPipeStreamAdapter<SslStream>


@sebastienros I'd like to run a performance test with this change. It could affect SSL performance.

davidfowl · 2019-06-20T13:31:50Z

The node services test exploded but things look good!

jkotalik · 2019-06-20T16:52:21Z

src/Servers/Kestrel/Core/src/Internal/AddressBinder.cs

@@ -171,8 +171,7 @@ public async Task BindAsync(AddressBindContext context)
                var httpsDefault = ParseAddress(Constants.DefaultServerHttpsAddress, out https);
                context.ServerOptions.ApplyEndpointDefaults(httpsDefault);

-                if (httpsDefault.ConnectionAdapters.Any(f => f.IsHttps)
-                    || httpsDefault.TryUseHttps())
+                if (httpsDefault.IsTls || httpsDefault.TryUseHttps())


Wasn't this necessary in our last PR? Do we not have anything testing this address strategy?

Before this change, ParseAddress would never return a ListenOptions object with a ConnectionAdapter pre-attached anyway. It looks like httpsDefault.ConnectionAdapters.Any(f => f.IsHttps) was never tru before and httpsDefault.IsTls is never true now.

More evidence of this is that the https out var is completely ignored.

Nevermind. I now see the ApplyEndpointDefaults line. My bad.

Was calling UseHttps in the ConfigureEndpointDefaults callback broken before this change?

jkotalik · 2019-06-20T16:53:03Z

src/Servers/Kestrel/Core/src/Internal/ConnectionDispatcher.cs

@@ -86,7 +86,7 @@ internal async Task Execute(KestrelConnection connection)
                    }
                    catch (Exception ex)
                    {
-                        Log.LogCritical(0, ex, $"{nameof(ConnectionDispatcher)}.{nameof(Execute)}() {connectionContext.ConnectionId}");
+                        Log.LogError(0, ex, "Unhandled exception while processing {ConnectionId}.", connectionContext.ConnectionId);


Why change this?

It's not a critical exception if middleware throws, it's just an error now. Any user code can cause this to happen.

src/Servers/Kestrel/Core/src/Internal/HttpsConnectionMiddleware.cs

src/Servers/Kestrel/Core/src/Middleware/Internal/DuplexPipeStreamAdapter.cs

src/Servers/Kestrel/Core/src/Middleware/Internal/LoggingConnectionMiddleware.cs

jkotalik · 2019-06-20T17:30:27Z

src/Servers/Kestrel/test/FunctionalTests/RequestTests.cs

@@ -509,7 +506,7 @@ public async Task AbortingTheConnectionSendsFIN()
        [MemberData(nameof(ConnectionAdapterData))]
        public async Task ConnectionClosedTokenFiresOnClientFIN(ListenOptions listenOptions)
        {
-            var testContext = new TestServiceContext(LoggerFactory);
+            var testContext = new TestServiceContext(LoggerFactory) { ExpectedConnectionMiddlewareCount = listenOptions._middleware.Count };


Should we remove asserting the middleware count? I feel like it's causing a lot of code churn with very little value.

I think we should remove it, but let @halter73 decide, he's the one that added it.

I'm fine with removing it. Before it was very unusual to have multiple, so it almost always signified a test bug where a ListenOptions object was being reused.

Now that so many tests add non-HttpConnectionMiddleware, I'm fine with removing it. Just watch out whenever you're writing kestrel tests not to reuse ListenOptions.

sebastienros · 2019-06-20T17:56:32Z

All good!

Citine/Windows

Before:

| Description |       RPS | CPU (%) | Memory (MB) | Avg. Latency (ms) | Startup (ms) | First Request (ms) | Latency (ms) | Errors |
| ----------- | --------- | ------- | ----------- | ----------------- | ------------ | ------------------ | ------------ | ------ |
|           √ | 2,481,298 |      95 |         411 |              1.34 |     412.5047 |           126.8114 |       0.1727 |      0 |
|           √ | 2,440,507 |      94 |         406 |              2.51 |     375.6787 |                  0 |            0 |      0 |
|           √ | 2,450,269 |      96 |         409 |              1.53 |     377.3042 |                  0 |            0 |      0 |

RequestsPerSecond:           2,457,358
Max CPU (%):                 95
WorkingSet (MB):             409
Avg. Latency (ms):           1.79
Startup (ms):                388
First Request (ms):          42.27
Latency (ms):                0.06
Total Requests:              37,104,666
Duration: (ms)               15,100
Socket Errors:               0
Bad Responses:               0
SDK:                         3.0.100-preview7-012531
Runtime:                     3.0.0-preview7-27812-08
ASP.NET Core:                3.0.0-preview7.19320.1

After:

| Description |       RPS | CPU (%) | Memory (MB) | Avg. Latency (ms) | Startup (ms) | First Request (ms) | Latency (ms) | Errors |
| ----------- | --------- | ------- | ----------- | ----------------- | ------------ | ------------------ | ------------ | ------ |
|           √ | 2,538,741 |      96 |         407 |              1.64 |      398.527 |           130.0194 |       0.1473 |      0 |
|           √ | 2,594,585 |      98 |         423 |              2.07 |     378.0464 |                  0 |            0 |      0 |
|           √ | 2,558,935 |      95 |         418 |              2.83 |     377.4521 |                  0 |            0 |      0 |

RequestsPerSecond:           2,564,087
Max CPU (%):                 96
WorkingSet (MB):             416
Avg. Latency (ms):           2.18
Startup (ms):                385
First Request (ms):          43.34
Latency (ms):                0.05
Total Requests:              38,701,248
Duration: (ms)               15,093
Socket Errors:               0
Bad Responses:               0
SDK:                         3.0.100-preview7-012531
Runtime:                     3.0.0-preview7-27812-08
ASP.NET Core:                3.0.0-preview7.19320.1

halter73 · 2019-06-20T21:58:11Z

src/Servers/Kestrel/test/FunctionalTests/ConnectionAdapterTests.cs

@@ -6,7 +6,6 @@
 using System.Net;


Nit: rename tests

halter73 · 2019-06-20T22:02:39Z

src/Servers/Kestrel/Core/src/Middleware/Internal/DuplexPipeStream.cs

 {
-    internal sealed class RawStream : Stream
+    internal class DuplexPipeStream : Stream


You finally renamed it!!! 🎉🎆🎉

halter73 · 2019-06-20T22:03:12Z

src/Servers/Kestrel/Core/src/Middleware/Internal/DuplexPipeStreamAdapter.cs

+    /// A helper for wrapping a Stream decorator from an <see cref="IDuplexPipe"/>.
+    /// </summary>
+    /// <typeparam name="TStream"></typeparam>
+    internal class DuplexPipeStreamAdapter<TStream> : DuplexPipeStream, IDuplexPipe where TStream : Stream


Are we planning to ship something like this in 3.1 in corefx?

I've informally spoken to a few people about it.

halter73 · 2019-06-20T22:04:57Z

src/Servers/Kestrel/Core/src/ListenOptions.cs

@@ -125,6 +110,13 @@ internal virtual string GetDisplayName()

        public override string ToString() => GetDisplayName();

+        /// <summary>
+        /// Adds a middleware delegate to the connection pipeline.
+        /// Configured by the <c>UseHttps()</c> and <see cref="Hosting.ListenOptionsConnectionLoggingExtensions.UseConnectionLogging(ListenOptions)"/>


Why no cref for UseHttps()?

- Remove ExpectedMiddlewareCount since everything is middleware now - Renamed everything adapter to middleware - Added a regression test for an https scenario - Don't send client certs for tests that don't expect it

Remove connection adapters and move things to middleware

4dfb1a1

- Remove connection adapters from the public API surface (pubternal) and replace the existing adapters with connection middleware. - Updated the tests

davidfowl requested review from analogrelay, halter73, jkotalik and Tratcher as code owners June 20, 2019 09:27

davidfowl commented Jun 20, 2019

View reviewed changes

Leave the underlying Stream open

559a266

analogrelay added breaking-change This issue / pr will introduce a breaking change, when resolved / merged. area-servers labels Jun 20, 2019

analogrelay added this to the 3.0.0-preview7 milestone Jun 20, 2019

jkotalik reviewed Jun 20, 2019

View reviewed changes

PR feedback and cleanup

8e783c0

halter73 reviewed Jun 20, 2019

View reviewed changes

src/Servers/Kestrel/test/FunctionalTests/ConnectionAdapterTests.cs

@@ -6,7 +6,6 @@

using System.Net;

Copy link

Member

halter73 Jun 20, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nit: rename tests

halter73 reviewed Jun 20, 2019

View reviewed changes

halter73 approved these changes Jun 20, 2019

View reviewed changes

davidfowl merged commit 25d5688 into master Jun 20, 2019

ghost deleted the davidfowl/remove-adapters branch June 20, 2019 22:26

davidfowl mentioned this pull request Jun 23, 2019

ConnectionAdapters have been removed from Kestrel aspnet/Announcements#371

Open

Tratcher mentioned this pull request Oct 2, 2019

ConnectionAdapters have been removed from Kestrel dotnet/docs#14905

Closed

amcasey added area-networking Includes servers, yarp, json patch, bedrock, websockets, http client factory, and http abstractions and removed area-runtime labels Jun 6, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Remove connection adapters and move things to middleware #11412

Remove connection adapters and move things to middleware #11412

davidfowl commented Jun 20, 2019

davidfowl Jun 20, 2019

davidfowl Jun 20, 2019

davidfowl Jun 20, 2019

davidfowl commented Jun 20, 2019

jkotalik Jun 20, 2019

halter73 Jun 20, 2019

halter73 Jun 20, 2019

halter73 Jun 20, 2019

jkotalik Jun 20, 2019

davidfowl Jun 20, 2019 •

edited

Loading

jkotalik Jun 20, 2019

davidfowl Jun 20, 2019

halter73 Jun 20, 2019

sebastienros commented Jun 20, 2019 •

edited

Loading

halter73 Jun 20, 2019

halter73 Jun 20, 2019

halter73 Jun 20, 2019

davidfowl Jun 20, 2019

halter73 Jun 20, 2019

Remove connection adapters and move things to middleware #11412

Remove connection adapters and move things to middleware #11412

Conversation

davidfowl commented Jun 20, 2019

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

davidfowl commented Jun 20, 2019

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

Choose a reason for hiding this comment

davidfowl Jun 20, 2019 • 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

sebastienros commented Jun 20, 2019 • 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

Choose a reason for hiding this comment

davidfowl Jun 20, 2019 •

edited

Loading

sebastienros commented Jun 20, 2019 •

edited

Loading