Add ISocketConnectionContextFactory

[HaoK]

Fixes #34522

Still trying to figure out why shutdown tests are failing, but want to see a full ci run

[davidfowl]

This changes the memory pool and socketsender lifetime. We still want one of those per endpoint.

[HaoK]

Guessing that's why the shutdown tests are all failing :) Yeah the analogy I used during standup today is that I feel like I've transplanted a kidney into a lung cavity, still figuring out how stuff works, thanks for the early feedback.

[HaoK]

This changes the memory pool and socketsender lifetime. We still want one of those per endpoint.

It wasn't clear to me how best to flow what is in the settings to the new SocketConnectionContextFactory when the API only had Create(Socket, SocketConnectionOptions). internal properties on the SocketConnectionOptions?

[HaoK]

Looks like all the tests are passing now except for the unix domain sockets, not sure what's different about those

There's a socket exception for the 2 unix socket tests: "An unknown, invalid, or unsupported option or level was specified in a getsockopt or setsockopt call."

(Looks like this is caused by accessing socket.NoDelay on unix sockets)

[HaoK]

I'm assuming I should also update

aspnetcore/src/Servers/Kestrel/Transport.Sockets/src/Client/SocketConnectionFactory.cs

Line 74 in bdafb7a

var socketConnection = new SocketConnection(

to use this new abstraction? At this point I might well update the SocketConncection ctor to just take the new options class

[halter73]

I'm assuming I should also update [SocketConnectionFactory.cs] to use this new abstraction?

I don't think so. This abstraction is for use by third-party transports not for allowing people to create their own ConnectionContexts from Sockets. I don't think anyone asked for the latter, so I'd rather not support that if we don't have to. I don't think SocketTransportFactory should consume the (I)SocketConnectionContextFactory either.

[HaoK]

I don't think anyone asked for the latter, so I'd rather not support that if we don't have to. I don't think SocketTransportFactory should consume the (I)SocketConnectionContextFactory either.

Okay I defer to you guys, just from the general purpose naming its a bit surprising that we wouldn't use it everywhere we create SocketConnectionContexts

[halter73]

Okay I defer to you guys, just from the general purpose naming its a bit surprising that we wouldn't use it everywhere we create SocketConnectionContexts

That's partly why I like removing the interface and not registering it with the IServiceCollection in UseKestrel(). In many cases, I expect it won't be registered as a service at all, but instead newed-up by a custom IConnectionListenerFactory with services it gets in its constructor.

When I filed the API proposal, I was considering an interface and not the concrete class. So we'll need to add the constructor to the API proposal. I think the ILoggerFactory would be the only parameter though.

[HaoK]

I don't think SocketTransportFactory should consume the (I)SocketConnectionContextFactory either

Right now the transport factory is who new's up the SocketConnectionListener in Bind so if it doesn't use it, no-one will since the socket connection listener isn't currently getting created from DI no?

[halter73]

Correct. This isn't for us to use. It's for third-party transports to use. So we'll definitely want to add some tests to make sure it all works end-to-end.

[HaoK]

That's partly why I like removing the interface and not registering it with the IServiceCollection in UseKestrel(). In many cases, I expect it won't be registered as a service at all, but instead newed-up by a custom IConnectionListenerFactory with services it gets in its constructor.

What's so bad about just treating this as a normal service that is responsible for creating ConnectionContexts from a Socket and SocketConnectionOptions? I'm obviously coming at this from a totally naïve viewpoint...that's what the current naming would have me expect its responsibility is

[halter73]

We have a lot more flexibility in the future we we don't consume the SocketConnectionContextFactory as a service in the socket transport.

If we consume it as a service, that effectively forces us to change a public API any time we want to change the SocketConnection constructor. Sometimes this might be warranted anyway so third-party transports using it always have the same capabilities we do, but that might not always be necessary if we just continue calling the constructor directly with the internal pools we manage and whatnot. I like being able to make this decision on a case-by-case basis rather than be force into it.

Anyone who's willing to do all the work to create a completely custom ConnectionContext from a Socket should just register their own IConnectionListenerFactory and bypass the socket transport completely since they've already done 90% of the work of implementing a custom IConnectionListenerFactory anyway. If someone just wants to decorate the ConnectionContext the socket transport would normally give you, they can use connection middleware and it'd work with any transport.

[halter73]

I've updated the issue with my current thinking about how the API should look. I should have never made it an interface or called it a service. The reason we decided not to make it a static method as originally proposed is not so it can be replaced and injested by the socket transport, but so it can keep pools around between calls.

[HaoK]

Pretty clean now, basically net changes look like we moved a bunch of code from the listener -> factory and split the transport options into two classes. Socket tests are green locally so I think this is ready for final review @halter73 @davidfowl

[davidfowl]

I wonder if this is a breaking change?

[HaoK]

It definitely affected the shipped public api's txt files

[HaoK]

Code check said no as well: Detected modification to baseline API files. PublicAPI.Shipped.txt files should only be updated after a major release. See /docs/APIBaselines.md for more information.

So back this out and just duplicate this in a new options class?

[davidfowl]

Yep

[halter73]

I don't think introducing a new base class by itself is a breaking change, but I'm pretty sure moving properties down to the new base class is. https://docs.microsoft.com/en-us/dotnet/core/compatibility/ says "Introducing a new base class" "REQUIRES JUDGMENT".

[HaoK]

Well I reverted that and just added an internal copy constructor to flow things to the 'base' ContextFactory options

[HaoK]

So we could do this in a non breaking way by making the properties virtual and override them to call base or something?

[davidfowl]

What you have is fine. Leave them separate.

[davidfowl]

This is super clean.

[davidfowl]

OK @kevin-montrose we re-did the change and make a class that can be created (without DI) that has all of the implementation guts of the transport. To enable your scenario, you can write a custom transport that handles the listening logic on both the incoming server and the server being routed to.

@mconnew for your scenario, we exposed the pipe settings so you can control the thresholds. We don't let you change the IDuplexPipe implementation before we read from the socket though. Hopefully that isn't a huge blocker. This should enable you to write a custom transport in CoreWCF without losing the behavior or performance enhancements we continue to make in the transport.