Obsolete KestrelServer (just the public type, not all of Kestrel!)

[JamesNK]

Kestrel is designed for DI. But there is a public way to create it directly: KestrelServer.cs

This is forcing various hacks on us:

• The real implementation of IServer is internal - KestrelServerImpl - and is wrapped by KestrelServer.
• Adding new DI types to KestrelServer forces us to consider how KestrelServer passes them to KestrelServerImpl. See metrics PR and Andrew's TLS PR.
• Falling behind on functionality. For example, KestrelServer doesn't support HTTP/3 (no ctor overload was added for specifying multiplexed transports).

Prior art in this area:

• IISHttpServer is internal.
• MessagePump is internal.
• TestServer is public.

I think we should obsolete KestrelServer and require creating it with a builder.

[Tratcher]

We should help fix this code:
https://github.com/natemcmaster/LettuceEncrypt/blob/2ca07d26249e9a7bcb4f7671d5fccbf61089c454/src/LettuceEncrypt/Internal/AcmeCertificateLoader.cs#L43

[adityamandaleeka]

Click-bait title! 🤣

[JamesNK]

I tried to make it less click bait.

[JamesNK]

We should help fix this code: natemcmaster/LettuceEncrypt@2ca07d2/src/LettuceEncrypt/Internal/AcmeCertificateLoader.cs#L43

Probably. But assuming KestrelServerImpl is the new IServer type then it should continue to work. It starts with KestrelServer.

[amcasey]

We should help fix this code: natemcmaster/LettuceEncrypt@2ca07d2/src/LettuceEncrypt/Internal/AcmeCertificateLoader.cs#L43

Probably. But assuming KestrelServerImpl is the new IServer type then it should continue to work. It starts with KestrelServer.

Given that the project is in maintenance mode, I'd be inclined to let it keep working "by accident" (though calling StartsWith seems like a very intentional choice?).

[amcasey]

So what's the process here? Create an API issue suggesting we add the attribute?

[Tratcher]

We already treat it as obsolete, adding the attribute seems fine.

I don't think an API issue is warranted. I'll just put the labels on this one.

[ghost]

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.

[halter73]

Probably. But assuming KestrelServerImpl is the new IServer type then it should continue to work. It starts with KestrelServer.

I think the workaround for anyone using nameof(KestrelServer) is to just hard code "KestrelSever" to avoid obsoletion warnings. Nothing guarantees that the real Kestrel IServer really starts with nameof(KestrelServer), so using a string literal should be no more fragile.

I also agree we should keep it working. There's no reason to rename the real Kestrel IServer to something that doesn't start with KestrelServer at this point.

So what's the process here? Create an API issue suggesting we add the attribute?

I think it's still worth filling out the API proposal template. I'll do it though.

Background and Motivation

See primary issue.

Proposed API

namespace Microsoft.AspNetCore.Server.Kestrel.Core;

+ [Obsolete]
public class KestrelServer : IServer
{
}

Usage Examples

Old usages of new KestrelServer(...) would be encouraged to use a host and resolve the IServer instead?

// Before (now obsolete)
var kestrelOptions = new KestrelServerOptions();
//  ...
var server = new KestrelServer(
    Options.Create(kestrelOptions),
    new SocketTransportFactory(Options.Create(new SocketTransportOptions()), new LoggerFactory()),
    new LoggerFactory());

// After
var server = new HostBuilder().ConfigureWebHost(webBuilder =>
{
    webBuilder.UseKestrel(kestrelOptions =>
    {
        // ...
    }
}).Build().Services.GetRequiredService<IServer>();

// Usage of IServer vs KestrelServer should be the same.

Alternative Designs

• Leave KestrelServer public.
• Add an AddKestrelServer extension method to IServiceCollection so you can use Kestrel as an IServer with just a DI container without needing to configure a Host.

Risks

• There are some usages of new KestrelServer(... which you can find with this query: https://grep.app/search?q=new%20KestrelServer%28. That code would need to be updated to use a host or at least a DI container which some people might be morally opposed to.

[halter73]

API Review Notes:

• @mconnew says he wants this API for CoreWCF.
  • It already uses it here.
  • Can we just use separate IServiceCollections/providers?
    • They want a shared DI container for multiple server instances.
    • Could Kestrel's IServer ServiceDescriptor be copied as transient? 😝
      • This was a joke, but it needs to work when IIS is the main IServer for the container.
• What if hosting just started every IServer instead? This could be super breaking for existing apps expecting the last one to win. See Running multiple IServer's #27213
• Instead should KestrelServer be the real IServer instead of KestrelServerImpl again?
  • It's annoying to make public API changes every time we consume a new service in Kestrel.
• If we did obsolete this, we would not delete it until @mconnew's scenario is addressed.
• The main issue this address is that this type does not support HTTP/3, and that seems worthwhile even though this isn't commonly used to begin with.

Blocked on #27213.

[ghost]

We've moved this issue to the Backlog milestone. This means that it is not going to be worked on for the coming release. We will reassess the backlog following the current release and consider this item at that time. To learn more about our issue management process and to have better expectation regarding different types of issues you can read our Triage Process.