Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Kestrel: Support for multiple transports, side-by-side #44537

Closed
1 task done
JamesNK opened this issue Oct 14, 2022 · 23 comments · Fixed by #44657
Closed
1 task done

Kestrel: Support for multiple transports, side-by-side #44537

JamesNK opened this issue Oct 14, 2022 · 23 comments · Fixed by #44657
Labels
api-approved API was approved in API review, it can be implemented area-networking Includes servers, yarp, json patch, bedrock, websockets, http client factory, and http abstractions

Comments

@JamesNK
Copy link
Member

JamesNK commented Oct 14, 2022

Is there an existing issue for this?

  • I have searched the existing issues

Is your feature request related to a problem? Please describe the problem.

Kestrel supports being extended to support new transports. This is done by registering IConnectionListenerFactory or IMultiplexedConnectionListenerFactory in DI.

When starting up, for each factory type, Kestrel then gets all registered factories. It then uses the last one for any registered listeners. IConnectionListenerFactory is used for HTTP/1.1 and HTTP/2 endpoints, and IMultiplexedConnectionListenerFactory is used for HTTP/3.

The problem with this is it isn't easily possible to mix transports together. Someone might want to support TCP endpoints using options.ListenLocalhost(80) and a custom transport using options.Listen(new MyCustomEndPoint("address")). Both endpoints are sent to the last factory type.

var builder = WebApplication.CreateBuilder(args);
builder.Services.AddGrpc();
builder.Services.AddSingleton<IConnectionListenerFactory, NamedPipeTransportFactory>();
builder.WebHost.ConfigureKestrel(options =>
{
    options.ListenLocalhost(80); // TCP!
    options.Listen(new NamedPipeEndPoint("Pipe-1")); // Named pipes!
});

var app = builder.Build();

Describe the solution you'd like

I think default interface implementation methods should be added to IConnectionListenerFactory and IMultiplexedConnectionListenerFactory which can be used to test if a factory supports binding a given endpoint.

public interface IConnectionListenerFactory
{
    Task<IConnectionListener> BindAsync(EndPoint endpoint, CancellationToken cancellationToken)
+   public bool CanBind(EndPoint endpoint) => true;
}
public interface IMultiplexedConnectionListenerFactory
{
    Task<IMultiplexedConnectionListener> BindAsync(EndPoint endpoint, CancellationToken cancellationToken)
+   public bool CanBind(EndPoint endpoint) => true;
}

Kestrel continues to get all registered instances of IConnectionListenerFactory and IMultiplexedConnectionListenerFactory from DI, but instead of using the last one registered, Kestrel now keeps all instances, and then when binding a new listener, Kestrel tests each factory with CanBind to see whether it supports binding a given endpoint, then falls back to the next listener.

The new CanBind default implementation returns true, which follows current behavior, and existing factories can override the CanBind method to only allow currently supported addresses. If no factories support an endpoint then Kestrel throws an error saying the endpoint isn't supported.

Additional context

IConnectionListenerFactory and IMultiplexedConnectionListenerFactory are in the Microsoft.AspNetCore.Connections.Abstractions project. It targets netstandard2.0 and .NET Framework. The new DIM would only be available in .NET 8+. Is there precedence for doing this?

@JamesNK
Copy link
Member Author

JamesNK commented Oct 14, 2022

@davidfowl
Copy link
Member

What happens if multiple can bind the endpoint type?

@JamesNK
Copy link
Member Author

JamesNK commented Oct 14, 2022

The last registered factory wins. That's what happens today:

_transportFactory = transportFactories.LastOrDefault();
_multiplexedTransportFactory = multiplexedFactories?.LastOrDefault();

In the sample below, SocketsTransportFactory is still in DI. Because NamedPipeTransportFactory was registered afterward, it is used for everything. A CanBind method gives an opportunity for unsupported endpoints, e.g. IPEndPoint, UnixDomainSocketEndPoint to fallback.

var builder = WebApplication.CreateBuilder(args);
builder.Services.AddGrpc();
builder.Services.AddSingleton<IConnectionListenerFactory, NamedPipeTransportFactory>(); // <- registered last
builder.WebHost.ConfigureKestrel(options =>
{
    options.Listen(new NamedPipeEndPoint("Pipe-1"), listenOptions =>
    {
        listenOptions.Protocols = HttpProtocols.Http2;
    });
});

var app = builder.Build();

If there are multiple factories that support the same endpoint, then the last one registered wins because CanBind stops falling back.

@davidfowl
Copy link
Member

So the logic is that we get all the transports, then we call canbind on all of the endpoints, and the last one that can bind wins?

@JamesNK
Copy link
Member Author

JamesNK commented Oct 14, 2022

// Reverse so the last registered transport has priority and attempts to bind first
_transportFactories = transportFactories.Reverse().ToList();

public async Task<IConnectionListener> BindAsync(Endpoint endpoint, CancellationToken cancellationToken)
{
    foreach (var factory in _transportFactories)
    {
        if (factory.CanBind(endpoint))
        {
            return await factory.BindAsync(endpoint, cancellationToken);
        }
    }

    throw new InvalidOperationException("Endpoint '{endpoint}' is not supported by any registered transport factory.");
}

@davidfowl
Copy link
Member

Or just loop backwards to avoid that allocation 😅

@Tratcher Tratcher added this to the .NET 8 Planning milestone Oct 14, 2022
@ghost
Copy link

ghost commented Oct 14, 2022

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
Copy link
Member

I like it. Should we label this as ready for API review?

@JamesNK
Copy link
Member Author

JamesNK commented Oct 15, 2022

Or just loop backwards to avoid that allocation 😅

There is a loop per-bind. Probably cheaper to reverse once and store it as a list and avoid IEnumerable allocation per bind 😋

@JamesNK JamesNK added the api-ready-for-review API is ready for formal API review - https://github.com/dotnet/apireviews label Oct 15, 2022
@ghost
Copy link

ghost commented Oct 15, 2022

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.

@davidfowl
Copy link
Member

There is a loop per-bind. Probably cheaper to reverse once and store it as a list and avoid IEnumerable allocation per bind 😋

dotnet/runtime#49970

@halter73
Copy link
Member

@JamesNK We decided to save this for an afternoon API review where hopefully you can make it. Do you want to propose APIs for NamedPipeEndpoint and NamedPipeTransportFactory as well?

@JamesNK
Copy link
Member Author

JamesNK commented Oct 18, 2022

#44612

@halter73
Copy link
Member

API Review Notes:

  1. This seems useful considering the abstraction supports multiple Endpoint types many not implemented by the default socket transport
  2. What if I want to have different transports listen on the same type of endpoint?
    • Can we have the endpoint pick the transport somehow?
    • Can we add an option to ListenOptions?
      • We could add a ListenOptions API later if this scenario is real. It's kinda hard to use. You don't always have a listen options callback.

It targets netstandard2.0 and .NET Framework. The new DIM would only be available in .NET 8+. Is there precedence for doing this?

  • We could #ifdef the API for .NET 8 and greater only.
  • We could add a new a new interface like ICanBindToEndpoint.
    • IConnectionListenerFactorySelector?
  • If the interface is missing, we assume the transport supports the endpoint.
namespace Microsoft.AspNetCore.Connections;

+ public interface IConnectionListenerFactorySelector
+ {
+     bool CanBind(EndPoint endpoint);
+ }

@halter73 halter73 added api-approved API was approved in API review, it can be implemented and removed api-ready-for-review API is ready for formal API review - https://github.com/dotnet/apireviews labels Oct 27, 2022
@mconnew
Copy link
Member

mconnew commented Oct 28, 2022

Is it worth looking at things at an even higher level than this? The scenario that CoreWCF needs to support is using Kestrel with a custom IConnectionListenerFactory to support NetTcp and NamedPipes, while being hosted in IIS and using the IIS Integration for HTTP requests. Eventually we'll need to go as far as adding support to the aspnetcoremodule for service activation of non-http protocols (or achieving this via a secondary IIS module that's installed). This approach is still going to result in needing to wire up the equivalent of Kestrel via an IHostedService when using IIS which seems like the wrong approach.

@JamesNK
Copy link
Member Author

JamesNK commented Oct 28, 2022

NetTcp is over a socket, right? What prevents CoreWCF + Kestrel from using the existing sockets transport factory? Likewise, what would prevent CoreWCF + Kestrel from using the new named pipes transport that we're adding in .NET 8?

@mconnew
Copy link
Member

mconnew commented Oct 28, 2022 via email

@davidfowl
Copy link
Member

@mconnew
Copy link
Member

mconnew commented Oct 28, 2022

@davidfowl, have you tested this in IIS? In CoreWCF we already replace the IServer with a wrapping one (to enable us to throw exceptions on service init as we need to init late due to IServerAddressesFeature) and this broke on IIS. The IIS integration piece throws an exception if the IServer is the wrong type so we have logic to not wrap on IIS (you don't get to see the exceptions on IIS anyway).

@mconnew
Copy link
Member

mconnew commented Oct 28, 2022

Here's the issue when someone hit this. The full call stack is there.

@mconnew
Copy link
Member

mconnew commented Nov 14, 2022

@davidfowl, do you have a suggestion on how to overcome the IIS problem? I'm still in the situation where I can't use Kestrel and IIS together and I'm about to add NamedPipe support to CoreWCF very soon. As things currently stand, I'm going to have to compose a lot more of the stack manually than is ideal and will be fragile to future asp.net core design changes under the hood.

@Tratcher
Copy link
Member

@mconnew that's not a case where you need multiple transports, but rather multiple servers. We've dabbled with this a bit, making an IServer that activates both instances.

@mconnew
Copy link
Member

mconnew commented Nov 24, 2022

@mconnew that's not a case where you need multiple transports, but rather multiple servers. We've dabbled with this a bit, making an IServer that activates both instances.

Supporting multiple IServer instances would solve the need to support different types of connection listeners. You would simply have multiple Kestrel IServer instances, one for each connection listener type. It would remove the need for this new proposed API, and would solve more problems at the same time.

@ghost ghost locked as resolved and limited conversation to collaborators Dec 24, 2022
@amcasey amcasey added area-networking Includes servers, yarp, json patch, bedrock, websockets, http client factory, and http abstractions and removed area-runtime labels Aug 25, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
api-approved API was approved in API review, it can be implemented area-networking Includes servers, yarp, json patch, bedrock, websockets, http client factory, and http abstractions
Projects
None yet
Development

Successfully merging a pull request may close this issue.

6 participants