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

Allow minimal host to be created without default HostBuilder behavior #46040

Merged
merged 7 commits into from
Jan 21, 2023

Conversation

eerhardt
Copy link
Member

This adds a new Hosting API to reduce startup and app size, and ensures the default behavior is NativeAOT compatible.

Fix #32485

Notes:

  1. I wasn't sure how to refactor SlimWebHostBuilder and GenericWebHostBuilder to share more code. If you have thoughts or ideas, please let me know. Should they inherit from a common base class?
  2. Similar question for the new WebApplicationBuilder constructor.

This adds a new Hosting API to reduce startup and app size, and ensures the default behavior is NativeAOT compatible.

Fix dotnet#32485
Refactor the WebHostBuilder classes to share more code.
@eerhardt
Copy link
Member Author

I think this PR is ready to be reviewed, if anyone wants to take a look.

It is breaking too much functionality that will need to be reimplemented from the Extensions.HostBuilder.
options =>
{
// We've already applied "ASPNETCORE_" environment variables to hosting config
options.SuppressEnvironmentConfiguration = true;
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We have not already applied "ASPNETCORE_" environment variables to hosting config in this code path. Only "DOTNET_". We should do both. I bet this is why the macOS template test is failing to load the server certificate.

We had an issue a while back with the template tests using expired dev certs on macOS. That was fixed by using "ASPNETCORE_"-prefixed environment variables to set a default cert, so the tests shouldn't rely on the development cert being up to date.

var finalEnvironmentVariables = new Dictionary<string, string>(environmentVariables)
{
["ASPNETCORE_Kestrel__Certificates__Default__Path"] = _developmentCertificate.CertificatePath,
["ASPNETCORE_Kestrel__Certificates__Default__Password"] = _developmentCertificate.CertificatePassword,
};

I'm guessing the dev cert is working as a fallback on the other OS's, but we still have that original issue on macOS when it falls back to the dev cert. @adityamandaleeka I wonder if this is related to our wider macOS dev cert issues.

Copy link
Member

@halter73 halter73 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Have you tested how it interacts with WebApplicationFactory? My assumption is that it will get the IHostBuilder from HostApplicationBuilder.HostBuilderAdapter and then add back the GenericWebHostBuilder rather than the SlimWebHostBuilder here.

hostBuilder.ConfigureWebHost(webHostBuilder =>
{
SetContentRoot(webHostBuilder);
_configuration(webHostBuilder);
webHostBuilder.UseTestServer();
});

Not a big deal in terms of this PR, but something to follow up on. I was talking to @davidfowl about creating a more "minimal" version of WebApplicationFactory, so that could tie in.

I'm also interested in seeing how much emptier we could possibly make this. I know there's a lot more trimming to do in lower layers even for the "slim" mode.

get
{
yield return new[] { (CreateBuilderFunc)CreateBuilder };
yield return new[] { (CreateBuilderFunc)CreateSlimBuilder };
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nit: How easy is it to determine which builder failed if only one does with this member data? It might be nicer to use strings for the MemberData and have it index into a static Dictionary<string, CreateBuilderFunc>. We are often bad at using serializable member data. But it might be worth it here given how often this is used.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This actually works pretty well, from what I can tell. In Test Explorer:

    2)   Microsoft.AspNetCore.Tests.WebApplicationTests.AddingMemoryStreamBackedConfigurationWorks(createBuilder: CreateBuilderFunc { Method = Microsoft.AspNetCore.Builder.WebApplicationBuilder CreateSlimBuilder(), Target = null }) 
      Duration: 1 ms

    1)   Microsoft.AspNetCore.Tests.WebApplicationTests.AddingMemoryStreamBackedConfigurationWorks(createBuilder: CreateBuilderFunc { Method = Microsoft.AspNetCore.Builder.WebApplicationBuilder CreateBuilder(), Target = null }) 
      Duration: 3 ms

And when the test fails:

image

Explicitly add ASPNETCORE environment variables.
@eerhardt eerhardt enabled auto-merge (squash) January 20, 2023 23:43
@eerhardt
Copy link
Member Author

Have you tested how it interacts with WebApplicationFactory?

No, I haven't. I don't really know much about it and can try it next week. Or I can log a follow up issue to track ensuring it works as we want it to.

I'm also interested in seeing how much emptier we could possibly make this. I know there's a lot more trimming to do in lower layers even for the "slim" mode.

Yeah, there is much more work to make it smaller. This is just the start to get the API in. Now other work items (one example: #46142) can build on this to trim more out.

My next plan is to get a new API into Extensions.Hosting for an "empty" HostApplicationBuilder. This code can then use that, and trim even more stuff out by default.

eerhardt added a commit to eerhardt/Benchmarks that referenced this pull request Jan 21, 2023
The `dotnet new api -aot` template is getting updated to use the new slim API in dotnet/aspnetcore#46040. Updating the benchmark to match.
@eerhardt eerhardt merged commit 0a3a01b into dotnet:main Jan 21, 2023
@ghost ghost added this to the 8.0-preview1 milestone Jan 21, 2023
@eerhardt eerhardt deleted the NewHostingAPI branch January 21, 2023 04:07
@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 join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area-networking Includes servers, yarp, json patch, bedrock, websockets, http client factory, and http abstractions
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Allow minimal host to be created without default HostBuilder behavior
3 participants