SocketTransportFactory should implement IConnectionListenerFactorySelector publically

[halter73]

Background and Motivation

This is to better support side-by-side transports using the new IConnectionListenerFactorySelector interface approved in #44537. This is already being implemented by #44832.

Proposed API

namespace Microsoft.AspNetCore.Server.Kestrel.Transport.Sockets;

- public sealed class SocketTransportFactory : IConnectionListenerFactory
+ public sealed class SocketTransportFactory : IConnectionListenerFactory, IConnectionListenerFactorySelector
{
+    public bool CanBind(EndPoint endpoint);
}

Usage Examples

This method already called by Kestrel with the change introduced by #44657. Here's how Kestrel now calls it:

public async Task<EndPoint> BindAsync(EndPoint endPoint, ConnectionDelegate connectionDelegate, EndpointConfig? endpointConfig, CancellationToken cancellationToken)
{
    if (_transportFactories.Count == 0)
    {
        throw new InvalidOperationException($"Cannot bind with {nameof(ConnectionDelegate)} no {nameof(IConnectionListenerFactory)} is registered.");
    }

    foreach (var transportFactory in _transportFactories)
    {
        var selector = transportFactory as IConnectionListenerFactorySelector;
        if (CanBindFactory(endPoint, selector))
        {
            var transport = await transportFactory.BindAsync(endPoint, cancellationToken).ConfigureAwait(false);
            StartAcceptLoop(new GenericConnectionListener(transport), c => connectionDelegate(c), endpointConfig);
            return transport.EndPoint;
        }
    }

    throw new InvalidOperationException($"No registered {nameof(IConnectionListenerFactory)} supports endpoint {endPoint.GetType().Name}: {endPoint}");
}

// ...

private static bool CanBindFactory(EndPoint endPoint, IConnectionListenerFactorySelector? selector)
{
    // By default, the last registered factory binds to the endpoint.
    // A factory can implement IConnectionListenerFactorySelector to decide whether it can bind to the endpoint.
    return selector?.CanBind(endPoint) ?? true;
}

Alternative Designs

We could implement the interface explicitly instead (e.g. bool IConnectionListenerFactorySelector.CanBind(Endpoint)), or not implement it at all since the socket transport is typically going to be the first transport registered and therefore last transport checked by Kestrel.

Risks

None that I can really think of. SocketTransportFactory is rarely used directly

Edit: @JamesNK has an interesting risk he brought up in the PR description of #44832.

A custom method creating the socket means potentially any endpoint could work. Inside user code, a custom EndPoint implementation 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.

Thank you for submitting this for API review. This will be reviewed by @dotnet/aspnet-api-review at the next meeting of the ASP.NET Core API Review group. Please ensure you take a look at the API review process documentation and ensure that:

• The PR contains changes to the reference-assembly that describe the API change. Or, you have included a snippet of reference-assembly-style code that illustrates the API change.
• The PR describes the impact to users, both positive (useful new APIs) and negative (breaking changes).
• Someone is assigned to "champion" this change in the meeting, and they understand the impact and design of the change.

[JamesNK]

FYI the same change is happening to the Quic transport.

[halter73]

Good point. QuicTransportFactory is internal, so it's not technically public API, but it's worth considering the implications.

Thanks for contacting us.

We're moving this issue to the .NET 8 Planning milestone for future evaluation / consideration. We would like to keep this around to collect more feedback, which can help us with prioritizing this work. We will re-evaluate this issue, during our next planning meeting(s).
If we later determine, that the issue has no community involvement, or it's very rare and low-impact issue, we will close it - so that the team can focus on more important and high impact issues.
To learn more about what to expect next and how this issue will be handled you can read more about our triage process here.

[halter73]

API Review Notes:

1. Do we want to implement this explicitly? No. SocketTransportFactory is an advance API, and this isn't adding much complexity.
2. This should improve user experience by allowing Kestrel to indicated the endpoint is unsupported by all transports instead of just failing to bind. It allows fallback.

namespace Microsoft.AspNetCore.Server.Kestrel.Transport.Sockets;

- public sealed class SocketTransportFactory : IConnectionListenerFactory
+ public sealed class SocketTransportFactory : IConnectionListenerFactory, IConnectionListenerFactorySelector
{
+    public bool CanBind(EndPoint endpoint);
}

API Approved!

[JamesNK]

Done with #44832