Implement new bedrock listener abstraction and re-plat Kestrel on top #10321

davidfowl · 2019-05-17T07:12:13Z

This is a massive set of changes to Kestrel to remove the existing pubternal transport layer and implement a public facing API for listeners and clients, see the details here #10308.

This change only has the server side pieces of the story as I don't want to add the client APIs without having ported SignalR to use them. Here are the highlights:

Transport.Abstractions is empty (will be removed in a separate PR as it requires removing it from a ton of places)
TransportConnection has been moved to Connection.Abstractions (we can decide if we need to consolidate with DefaultConnectionContext in a later PR)
Added FileHandleEndPoint which allows binding to a file handle (could be a pipe or tcp handle)
ListenOptions has been gutted for most pubternal API and returns various types of binding information . The source of truth is the EndPoint instance.
Cleaned up a bunch of libuv tests decoupling them from Kestrel.Core

Breaking Changes

Removing pubternal API is itself a breaking change but one that we already planned to do.
We've removed the ability to set the scheduling mode on Kestrel, this will be a transport knob instead (77d3c27) cc @sebastienros (I know our benchmarks support this)
DisposeAsync was added to ConnectionContext (it will implement IAsyncDisposable in a later change, it supports netstandard 2.0 so that requires a bit more work) cc @anurse @BrennanConroy as this affects SignalR (4977f1b)
NoDelay is a noop, on ListenOptions, we should consider deprecating it. This has been moved to each of the transports. One major difference though is that it's no longer localized per endpoint but is global. We'd need a derived EndPoint type (or maybe extend IPEndPoint) to store both the socket options and the binding information.

@halter73 this is ready for review now.

Fixes #10308
Fixes #4752

gfoidl · 2019-05-17T14:13:40Z

src/Servers/Connections.Abstractions/src/IConnectionListener.cs

+    {
+        ValueTask<ConnectionContext> AcceptAsync();
+
+        ValueTask DisposeAsync();


Should the interface be bassed on IAsyncDisposable?
Same above in ConnectionContext.

Yes I just hacked it because this is ns2.0.

src/Servers/Kestrel/Core/src/KestrelServer.cs

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

src/Servers/Kestrel/Transport.Libuv/src/Internal/LibuvConnectionListener.cs

src/Servers/Kestrel/Transport.Libuv/src/Internal/ListenerContext.cs

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

src/Servers/Connections.Abstractions/src/ConnectionContext.cs

src/Servers/Kestrel/Transport.Sockets/src/Internal/SocketConnection.cs

src/Servers/Kestrel/Transport.Sockets/src/SocketConnectionListener.cs

src/Servers/Kestrel/Core/src/Internal/Infrastructure/KestrelConnection.cs

src/Servers/Kestrel/Core/test/ConnectionDispatcherTests.cs

davidfowl · 2019-05-26T07:27:45Z

src/SignalR/clients/csharp/Http.Connections.Client/src/HttpConnection.cs

        /// a new instance should be created using the same options.
        /// </remarks>
-        public async Task DisposeAsync()
+        public override async ValueTask DisposeAsync()


cc @BrennanConroy @anurse

davidfowl · 2019-05-27T01:16:37Z

/azp run

azure-pipelines · 2019-05-27T01:16:42Z

Pull request contains merge conflicts.

davidfowl · 2019-05-27T01:18:37Z

AHHHHH

- Added NoDelay to transport options - Added FileHandleEndPoint to represent listening to a file handle

- Moved feature implementations into KestrelConnection out of TransportConnection

halter73 · 2019-05-29T06:46:14Z

src/SignalR/SignalR.sln

 EndProject
 Project("{9A19103F-16F7-4668-BE54-9A1E7A4F7556}") = "Microsoft.AspNetCore.SignalR.Protocols.Json", "common\Protocols.Json\src\Microsoft.AspNetCore.SignalR.Protocols.Json.csproj", "{BB52C0FB-19FD-485A-9EBD-3FC173ECAEA0}"
 EndProject
+Project("{9A19103F-16F7-4668-BE54-9A1E7A4F7556}") = "Microsoft.AspNetCore.Metadata", "..\Http\Metadata\src\Microsoft.AspNetCore.Metadata.csproj", "{2E107FBB-2387-4A9F-A2CA-EFDF2E4DD64D}"


You're adding IAllowAnonymous and IAuthorizeData to the SignalR solution in this PR?

I needed it to compile, so pretend its ok. We added a new package but didn't add it to all the places. I had to make sure SignalR was working so I included this in the PR

src/Servers/Kestrel/Transport.Sockets/src/Internal/SocketConnection.cs

src/Servers/Kestrel/Transport.Libuv/src/Internal/LibuvConnection.cs

davidfowl · 2019-05-29T07:01:36Z

/azp run

azure-pipelines · 2019-05-29T07:02:09Z

Azure Pipelines successfully started running 3 pipeline(s).

halter73

Approved pending finding and fixing the root cause of the UvLoopHandle finalized. It would also be nice if you wrote "Primary"/"Secondary" to the accepted pipes in ListenerPrimaryTests and verified the response.

- Added ConfigureAwait(false) to DisposeAsync path - Added await using in tests to make sure threads are disposed - Remove more dead code

davidfowl · 2019-05-30T07:56:55Z

Approved pending finding and fixing the root cause of the UvLoopHandle finalized.

OK I spent a few hours on this and I'm still stumped and haven't been able to write a test to cause the condition to happen. I make a few tweaks to use await using in a couple of places just to make sure we're calling dispose in the right spots.

What if we just add a log here so that we can know if this is a resource leak or a bad unit test?

- Add a method to quickly run through each of the queued aborting all connections instead of using the IAsyncEnumerator.

davidfowl · 2019-05-30T16:22:25Z

/azp run

azure-pipelines · 2019-05-30T16:23:04Z

Azure Pipelines successfully started running 3 pipeline(s).

halter73 · 2019-05-30T17:25:23Z

What if we just add a log here so that we can know if this is a resource leak or a bad unit test?

Seems reasonable. We should probably log something anytime OnStopRude() let alone OnStopImmediate() is called. Maybe we could make those logs fail tests like the ungraceful shutdown log does.

jkotalik · 2019-05-30T22:38:36Z

src/Servers/Kestrel/Transport.Sockets/src/SocketTransportOptions.cs

+        /// </remarks>
+        public bool NoDelay { get; set; } = true;
+
+        public long? MaxReadBufferSize { get; set; } = PipeOptions.Default.PauseWriterThreshold;


Nit: doc comments?

davidfowl requested a review from halter73 May 17, 2019 07:12

gfoidl reviewed May 17, 2019

View reviewed changes

Eilon added the area-servers label May 17, 2019

ReubenBond reviewed May 17, 2019

View reviewed changes

src/Servers/Kestrel/Core/src/Internal/ConnectionDispatcher.cs Show resolved Hide resolved

ReubenBond reviewed May 17, 2019

View reviewed changes

src/Servers/Connections.Abstractions/src/ConnectionContext.cs Show resolved Hide resolved

davidfowl commented May 19, 2019

View reviewed changes

src/Servers/Kestrel/Transport.Sockets/src/Internal/SocketConnection.cs Outdated Show resolved Hide resolved

davidfowl commented May 19, 2019

View reviewed changes

src/Servers/Kestrel/Transport.Sockets/src/SocketConnectionListener.cs Outdated Show resolved Hide resolved

davidfowl commented May 23, 2019

View reviewed changes

src/Servers/Kestrel/Core/src/Internal/Infrastructure/KestrelConnection.cs Show resolved Hide resolved

davidfowl commented May 24, 2019

View reviewed changes

src/Servers/Kestrel/Core/test/ConnectionDispatcherTests.cs Show resolved Hide resolved

davidfowl commented May 26, 2019

View reviewed changes

davidfowl added 17 commits May 26, 2019 18:56

Spike Kestrel with new transport abstraction.

42622b7

Libuv works

5d91d92

Removed features

b4e7ac8

Added EndPoint to IConnectionListener

a605520

Remove null check

582cbbf

Null means no more connections

6d539b1

More cleanup

2c797d0

- Added NoDelay to transport options - Added FileHandleEndPoint to represent listening to a file handle

Endpoint -> EndPoint

daec056

Generate the connection id in kestrel

a2281c2

Implement shutdown in the socket transport

c471ce0

Progress towards removing the task

3f864d4

Remove the KestrelConnection cast

60efcbd

- Moved feature implementations into KestrelConnection out of TransportConnection

Remove IHttpConnectionFeature

17fa5fc

Remove comment

789ae16

Fix in memory transport

edf0557

Make more tests compile

c1f9f13

Use the memory pool

834811f

halter73 reviewed May 29, 2019

View reviewed changes

src/Servers/Kestrel/Transport.Sockets/src/Internal/SocketConnection.cs Outdated Show resolved Hide resolved

halter73 reviewed May 29, 2019

View reviewed changes

src/Servers/Kestrel/Transport.Libuv/src/Internal/LibuvConnection.cs Show resolved Hide resolved

davidfowl added 2 commits May 29, 2019 01:25

PR feedback

e488e07

Handle connection reset

9e341c3

halter73 approved these changes May 29, 2019

View reviewed changes

davidfowl added 2 commits May 29, 2019 23:25

Some cleanup

21ac350

- Added ConfigureAwait(false) to DisposeAsync path - Added await using in tests to make sure threads are disposed - Remove more dead code

Make everything async and clean up the accept loop

940e6fa

Improve aborting queued connection logic

1cec23c

- Add a method to quickly run through each of the queued aborting all connections instead of using the IAsyncEnumerator.

jkotalik reviewed May 30, 2019

View reviewed changes

davidfowl added 2 commits May 30, 2019 16:05

Added logs

c4cdab0

Log critical for ungraceful loop shutdown

74868c1

davidfowl merged commit 04bf1bf into master May 31, 2019

ghost deleted the davidfowl/transport-refactor branch May 31, 2019 03:34

davidfowl mentioned this pull request Jun 1, 2019

Azure SignalR broken in preview6 Azure/azure-signalr#517

Closed

halter73 mentioned this pull request Aug 9, 2019

Update for ASP.NET Core 3.0 compat redhat-developer/kestrel-linux-transport#80

Merged

dodyg mentioned this pull request Sep 30, 2019

Track Project Bedrock dodyg/practical-aspnetcore#154

Open

halter73 mentioned this pull request Feb 4, 2020

Replat on System.Net.Quic #18689

Merged

BrennanConroy mentioned this pull request Sep 6, 2022

What are we doing with IConnectionEndPointFeature #43786

Open

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

PaulusParssinen mentioned this pull request Jun 10, 2023

Project Bedrock ArachisH/Sulakore#51

Open

Copilot AI mentioned this pull request May 29, 2025

Implement IConnectionEndPointFeature in Kestrel's TransportConnection #62162

Draft

Implement new bedrock listener abstraction and re-plat Kestrel on top #10321

Implement new bedrock listener abstraction and re-plat Kestrel on top #10321

Uh oh!

Conversation

davidfowl commented May 17, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Breaking Changes

Uh oh!

gfoidl May 17, 2019

Choose a reason for hiding this comment

Uh oh!

davidfowl May 17, 2019

Choose a reason for hiding this comment

Uh oh!

gfoidl May 17, 2019

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

davidfowl May 26, 2019

Choose a reason for hiding this comment

Uh oh!

davidfowl commented May 27, 2019

Uh oh!

azure-pipelines bot commented May 27, 2019

Uh oh!

davidfowl commented May 27, 2019

Uh oh!

halter73 May 29, 2019

Choose a reason for hiding this comment

Uh oh!

davidfowl May 29, 2019

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

davidfowl commented May 29, 2019

Uh oh!

azure-pipelines bot commented May 29, 2019

Uh oh!

halter73 left a comment

Choose a reason for hiding this comment

Uh oh!

davidfowl commented May 30, 2019

Uh oh!

davidfowl commented May 30, 2019

Uh oh!

azure-pipelines bot commented May 30, 2019

Uh oh!

halter73 commented May 30, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

jkotalik May 30, 2019

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

12 participants

davidfowl commented May 17, 2019 •

edited

Loading

halter73 commented May 30, 2019 •

edited

Loading