Add transport selector interface to Kestrel sockets and QUIC transports #44832
Conversation
JamesNK
requested review from
Tratcher,
halter73 and
BrennanConroy
as code owners
JamesNK
changed the title
| @@ -40,4 +41,16 @@ public ValueTask<IConnectionListener> BindAsync(EndPoint endpoint, CancellationT | |||
| transport.Bind(); | |||
| return new ValueTask<IConnectionListener>(transport); | |||
| } | |||
|
|
|||
| /// <inheritdoc /> | |||
| public bool CanBind(EndPoint endpoint) | |||
There was a problem hiding this comment.
This could be an explicit interface implementation if we don't want it public.
There was a problem hiding this comment.
Does anyone do this? Probably not. If someone is using a custom endpoint type to pass in custom values to the factory method, a workaround to this change is to inherit from one of the supported endpoint types, e.g.
IPEndPoint, and add new members..
Would another workaround be to make CanBind virtual so they can derive from the transport and override it?
There was a problem hiding this comment.
Implementing a new interface on a public type is an API change with or without an explicit interface implementation. It doesn't matter what our PublicAPI.Unshipped.txt files have to say about it.
Personally, I think we should make CanBind fully public. It's not like it adds a lot of clutter to the AIP surface of SocketTransportFactory.
There was a problem hiding this comment.
Sure, CanBind can stay public.
I don't like making CanBind virtual. Nothing else on this type is designed to be extended with inheritance. For example, the create socket factory method is configured via options as a func. If we want to allow people to customize CanBind, then it should also be a func on the options.
There was a problem hiding this comment.
I opened #45035 so we can discuss it in API review. I like it though.
| @@ -40,4 +41,16 @@ public ValueTask<IConnectionListener> BindAsync(EndPoint endpoint, CancellationT | |||
| transport.Bind(); | |||
| return new ValueTask<IConnectionListener>(transport); | |||
| } | |||
|
|
|||
| /// <inheritdoc /> | |||
| public bool CanBind(EndPoint endpoint) | |||
There was a problem hiding this comment.
I opened #45035 so we can discuss it in API review. I like it though.
|
The changed test error in this PR shows it works. |
ghost
added this to the 
amcasey
added
area-networking
Follow up PR to address comment #44657 (comment)
Note:
An edge case with
SocketTransportFactoryis it allows the factory method to be customized with SocketTransportOptions.CreateBoundListenSocket.A custom method creating the socket means potentially any endpoint could work. Inside user code, a custom
EndPointimplementation could be changed to an endpoint type supported by sockets. Limiting the endpoints that can be used to only built-in types would be a breaking change because the custom endpoint type would no longer work.Does anyone do this? Probably not. If someone is using a custom endpoint type to pass in custom values to the factory method, a workaround to this change is to inherit from one of the supported endpoint types, e.g.
IPEndPoint, and add new members..Is it worth calling out this change? I can create a breaking change issue for this situation after .NET 7 ships.