Consider letting the transport own the creation of IDuplexPipe

[davidfowl]

Today the transport is responsible for exposing properties on the connection that can be used by Kestrel to create a duplex pipe. It does this today for a few reasons:

• It wants to directly apply the MaxResponseBufferSize and MaxRequestBuffer size to pipe options
• It communicates "fin" via cancelling the transport's output reader (this is super hacky and we should get rid of it anyways).
• It wants to control where "user code" runs. This includes:
  • ConnectionAdapters (soon to be deprecated)
  • CancellationToken callbacks
  • the request processing delegate

Delegating to the transport has some other benefits:

• Transports aren't tied to a specific pipe implementation.
• We can potentially delete ITransportSchedulerFeature, IApplicationTransportFeature

To make this change though, we need to pass the relevant settings from kestrel to the transport without coupling. My current though is to pass a settings object to ITransportFactory.Create.

See https://github.com/geoffkizer/KestrelHttpServer/blob/7bd21cdb1581fc3559479842262a24deecf171bb/src/Kestrel.Transport.Sockets/Internal/SocketConnection.cs for an example of the socket transport with a direct pipe implementation.

/cc @tmds @benaadams (only other people that wrote transports 😄)

[tmds]

Probably it also means removing ApplicationSchedulingMode which is good to avoid double dispatch to the ThreadPool (e.g. Sockets).

I assume existing implementations can just create the two Pipes similar to how Kestrel does it now, and things will work as before.

[davidfowl]

Probably it also means removing ApplicationSchedulingMode which is good to avoid double dispatch to the ThreadPool (e.g. Sockets).

Not necessarily, the application callback (process request) could still be dispatched by kestrel itself.

I assume existing implementations can just create the two Pipes similar to how Kestrel does it now, and things will work as before.

That's correct.

[tmds]

Not necessarily, the application callback (process request) could still be dispatched by kestrel itself.

Ah, ok. I see the default is to use the ThreadPool:

https://github.com/aspnet/KestrelHttpServer/blob/af636fc8d4870aeba5b892cdcd3994a5d956c475/src/Kestrel.Core/KestrelServer.cs#L86-L88

So Sockets are doing a double dispatch currently.

[halter73]

So Sockets are doing a double dispatch currently.

Yep. That's why we were surprised that it was helpful to dispatch in SocketAsyncEventArgs.Completed on Windows.

[tmds]

@halter73 I didn't realize Windows also did a threadpool dispatch on Windows. When you benchmark Sockets with SchedulingMode.Inline what difference do you see with the default (ThreadPool)?

[davidfowl]

@halter73 I didn't realize Windows also did a threadpool dispatch on Windows

It doesn't, we dispatch and it performs much better than running inline

[tmds]

It doesn't, we dispatch and it performs much better than running inline

@davidfowl are you sure? In corefx I see the iocp stuff is using ThreadPoolBoundHandle, that sounds as if it is using the ThreadPool.

[halter73]

I'm sure we dispatch ourselves on Windows in SocketAwaitable.Complete(). ThreadPoolBoundHandle uses the ThreadPool to handle GetQueuedCompletionStatus on completion port threads, but I don't see anywhere it dispatches to worker threads on its own.

[tmds]

ThreadPoolBoundHandle uses the ThreadPool to handle GetQueuedCompletionStatus on completion port threads

ok, I don't understand iocp and I believe you when you say it doesn't run on the ThreadPool 😄

[muratg]

@davidfowl are you going to be working on this? If not, please punt. Thanks.

[davidfowl]

Acceptance checklist (check one item)

• We decided not to take this fix.
• The fix is tests-only.
• The fix contains product changes (check all items below).
  • Relevant XML documentation comments for new public APIs are present.
  • Narrative docs (docs.microsoft.com) are updated. (check one item below)
    • The change requires a new article. An issue with an outline has been filed here: [ISSUE LINK]
    • The change requires a change to an existing article. A docs PR with these changes is linked here: [PR LINK]
    • The change requires no docs changes.
  • Verification has been completed. (check one item below)
    • The change is in the shared framework and was verified against the following versions
      • SDK installer: [VERSION]
      • ASP.NET Core Runtime: [VERSION]
      • .NET Core Runtime: [VERSION]
    • The change is in an OOB NuGet/NPM/JAR package and was verified against the following version of that package: [PACKAGE ID] [VERSION]

[davidfowl]

We changed the design such that the pipes are owned by the transport as part of #10308