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

Improve HostBuilder and WebApplicationBuilder Configuration integration #61634

Closed
Tracked by #64015
halter73 opened this issue Nov 15, 2021 · 23 comments
Closed
Tracked by #64015

Improve HostBuilder and WebApplicationBuilder Configuration integration #61634

halter73 opened this issue Nov 15, 2021 · 23 comments
Assignees
Labels
api-approved API was approved in API review, it can be implemented area-Extensions-Hosting
Milestone

Comments

@halter73
Copy link
Member

halter73 commented Nov 15, 2021

Today WebApplicationBuilder has some hacky logic to synchronize the WebApplicationBuilder.Configuration ConfigurationManager and the HostBuilder's app IConfiguration in case any custom configuration sources have been added via HostFactoryResolver. This logic can be found here.

The synchronization is not perfect and can lead to bugs like dotnet/aspnetcore#38185. We should look into adding an API to HostBuilder to share IConfigurationBuilder(s) with the WebApplicationBuilder.

@halter73 halter73 added api-suggestion Early API idea and discussion, it is NOT ready for implementation area-Extensions-Hosting labels Nov 15, 2021
@dotnet-issue-labeler dotnet-issue-labeler bot added the untriaged New issue has not been triaged by the area owner label Nov 15, 2021
@ghost
Copy link

ghost commented Nov 15, 2021

Tagging subscribers to this area: @eerhardt, @maryamariyan
See info in area-owners.md if you want to be subscribed.

Issue Details

Today WebApplicationBuilder has some hacky logic to synchronize the WebApplicationBuilder.Configuration ConfigurationManager and the HostBuilder's app IConfiguration in case any custom configuration sources have been added via HostFactoryResolver. This logic can be found here.

The synchronization is not perfect and can lead to bugs like dotnet/aspnetcore#38185. We should look into adding an API to HostBuilder to share an IConfigurationBuilder with the WebApplicationBuilder.

Author: halter73
Assignees: -
Labels:

api-suggestion, area-Extensions-Hosting

Milestone: -

@maryamariyan maryamariyan removed the untriaged New issue has not been triaged by the area owner label Nov 15, 2021
@maryamariyan maryamariyan added this to the 7.0.0 milestone Nov 15, 2021
@eerhardt
Copy link
Member

@halter73 - Do you have an idea of what the API would look like? Can you update the issue to use the API proposal template?

https://github.com/dotnet/runtime/issues/new?assignees=&labels=api-suggestion&template=02_api_proposal.yml&title=%5BAPI+Proposal%5D%3A+

@halter73
Copy link
Member Author

halter73 commented Feb 1, 2022

Here's an API proposal that's an alternative to IHostBuilder. It would be a better base for WebApplicationBuilder then the IHostBuilder interface, but I think it all-around a more convenient API than IHostBuilder because it relies less on multi-stage config (instead leveraging ConfigurationManager) and callbacks that are run during build.

diff --git a/src/libraries/Microsoft.Extensions.Hosting/ref/Microsoft.Extensions.Hosting.cs b/src/libraries/Microsoft.Extensions.Hosting/ref/Microsoft.Extensions.Hosting.cs
index 742dde5a32a..0434b96536f 100644
--- a/src/libraries/Microsoft.Extensions.Hosting/ref/Microsoft.Extensions.Hosting.cs
+++ b/src/libraries/Microsoft.Extensions.Hosting/ref/Microsoft.Extensions.Hosting.cs
@@ -25,9 +25,31 @@ public partial class ConsoleLifetimeOptions
     }
     public static partial class Host
     {
+        public static Microsoft.Extensions.Hosting.HostApplicationBuilder CreateApplicationBuilder() { throw null; }
+        public static Microsoft.Extensions.Hosting.HostApplicationBuilder CreateApplicationBuilder(string[] args) { throw null; }
         public static Microsoft.Extensions.Hosting.IHostBuilder CreateDefaultBuilder() { throw null; }
         public static Microsoft.Extensions.Hosting.IHostBuilder CreateDefaultBuilder(string[] args) { throw null; }
     }
+    public partial class HostApplicationBuilder
+    {
+        public HostApplicationBuilder(Microsoft.Extensions.Hosting.HostApplicationOptions options) { }
+        public Microsoft.Extensions.Configuration.ConfigurationManager Configuration { get { throw null; } }
+        public Microsoft.Extensions.Hosting.IHostEnvironment Environment { get { throw null; } }
+        public Microsoft.Extensions.Logging.ILoggingBuilder Logging { get { throw null; } }
+        public Microsoft.Extensions.DependencyInjection.IServiceCollection Services { get { throw null; } }
+        public Microsoft.Extensions.Hosting.IHost Build() { throw null; }
+        public Microsoft.Extensions.Hosting.IHost Build<TContainerBuilder>(Microsoft.Extensions.DependencyInjection.IServiceProviderFactory<TContainerBuilder> serviceProviderFactory) { throw null; }
+    }
+    public partial class HostApplicationOptions
+    {
+        public HostApplicationOptions() { }
+        public string ApplicationName { get { throw null; } set { } }
+        public string[] Args { get { throw null; } set { } }
+        public string ContentRootPath { get { throw null; } set { } }
+        public bool DisableDefaults { get { throw null; } set { } }
+        public string EnvironmentName { get { throw null; } set { } }
+        public Microsoft.Extensions.Configuration.ConfigurationManager InitialConfiguration { get { throw null; } set { } }
+    }
     public partial class HostBuilder : Microsoft.Extensions.Hosting.IHostBuilder
     {
         public HostBuilder() { }

Sample Usage

var builder = Host.CreateApplicationBuilder(args);

builder.Configuration.AddJsonFile("myconfig.json");
Console.WriteLine($"MyConfigVariable = ${builder.Configuration["MyConfigVariable"]}");

builder.Services.AddHostedService<MyHostedService>();

var minConsoleLogLevel = builder.Environment.IsDevelopment() ? LogLevel.Trace : LogLevel.Warning;
builder.Logging.AddFilter<ConsoleLoggerProvider>(level => level >= minConsoleLogLevel);

builder.Build().Run();

Equivalent HostBuilder Sample

var builder = Host.CreateDefaultBuilder(args)
    .ConfigureAppConfiguration(hostConfig =>
    {
        hostConfig.AddJsonFile("myconfig.json");
    })
    .ConfigureServices((context, services) =>
    {
        Console.WriteLine($"MyConfigVariable = ${context.Configuration["MyConfigVariable"]}");
        services.AddHostedService<MyHostedService>();
    })
    .ConfigureLogging((context, logging) =>
    {
        var minConsoleLogLevel = context.HostingEnvironment.IsDevelopment() ? LogLevel.Trace : LogLevel.Warning;
        logging.AddFilter<ConsoleLoggerProvider>(level => level >= minConsoleLogLevel);
    });

builder.Build().Run();

I've started experimenting with this API here. And you can see how it might be consumed from WebApplicationBuilder here. It seems like we can share a fair amount of logic with the existing HostBuilder and share the same Host. I'm going to close #61635 because it's resolved by the same API.

The name of the new types are HostApplicationBuilder and HostApplicationOptions to somewhat align with similar concepts in ASP.NET (WebApplicationBuilder and WebApplicationOptions). I welcome better name recommendations.

@ladeak
Copy link
Contributor

ladeak commented Feb 3, 2022

I have been looking at the implementation of WebApplicationBuilder from Asp.Net Core, to see how the sync can be done.

I would like to apply the same logic for Console Apps using GenericHost. What I see is that I would need to copy multiple times the sources back and forth between the Generic Host's configuration builder and ConfigurationManager.

@halter73 halter73 added api-ready-for-review API is ready for review, it is NOT ready for implementation blocking Marks issues that we want to fast track in order to unblock other important work and removed api-suggestion Early API idea and discussion, it is NOT ready for implementation labels Feb 7, 2022
@terrajobst
Copy link
Member

terrajobst commented Feb 8, 2022

Video

  • Consider making the constructor-based flow for HostApplicationBuilder the "canonical" way moving forward
    • That doesn't mean we should not have the factory methods on Host to aid discoverability for existing code, but it would mean that we think about the constructors not as an advanced thing, but as something that should be usable by itself; e.g. maybe we should have one that takes string[] args.
  • We'd prefer to drop Host from HostApplicationBuilder but ASP.NET Core already has IApplicationBuilder that wouldn't be implemented by this type, which would be confusing.
  • HostApplicationBuilder doesn't implement IHostBuilder which means non of the existing methods will work.
    • Based on what @halter73 said, we don't believe this to be a problem and in fact wouldn't want this due to mismatches in capabilities.
    • @davidfowl suggested to look at GitHub and see what UseXxx extensions people actually have
  • We haven't really used the host builder pattern outside of web; MAUI has just started. We should validate that the concepts hold water outside of web.
  • @davidfowl has concerns that Build() taking the factory might not work
    • Suggestion was to replace that with the other existing pattern ConfigureContainer<TBuilder>()
  • It was suggested that HostApplicationBuilderSettings.Configuration should be IConfiguration but the idea is that HostApplicationBuilder will be set to that instance if it's provided, hence it makes more sense to be typed as ConfigurationManager.
  • We need to figure out how ASP.NET Core will use HostApplicationBuilder will it derive from it or will it wrap it? Based on that we should either make this type sealed or inheritable.
namespace Microsoft.Extensions.Hosting
{
     public static partial class Host
     {
        public static HostApplicationBuilder CreateApplicationBuilder();
        public static HostApplicationBuilder CreateApplicationBuilder(string[] args);
        public static IHostBuilder CreateDefaultBuilder();
        public static IHostBuilder CreateDefaultBuilder(string[] args);
    }
    public sealed class HostApplicationBuilder
    {
        public HostApplicationBuilder();
        public HostApplicationBuilder(string[] args);
        public HostApplicationBuilder(HostApplicationBuilderSettings settings);
        public ConfigurationManager Configuration { get; }
        public IHostEnvironment Environment { get; }
        public ILoggingBuilder Logging { get; }
        public IServiceCollection Services { get; }
        public IHost Build();
        public void ConfigureContainer<TBuilder>(IServiceProviderFactory<TBuilder> factory, Action<TBuilder>? configure = null) where TBuilder : notnull;
    }
    public sealed class HostApplicationBuilderSettings
    {
        public HostApplicationBuilderSettings();
        public string ApplicationName { get; set; }
        public string[] Args { get; set; }
        public string ContentRootPath { get; set; }
        public bool DisableDefaults { get; set; }
        public string EnvironmentName { get; set; }
        public ConfigurationManager Configuration { get; set; }
    }
}

@terrajobst terrajobst added api-approved API was approved in API review, it can be implemented and removed api-ready-for-review API is ready for review, it is NOT ready for implementation labels Feb 8, 2022
@eerhardt
Copy link
Member

With the new HostApplicationBuilder, we will need to also update

public static IHostBuilder UseSystemd(this IHostBuilder hostBuilder)

and

public static IHostBuilder UseWindowsService(this IHostBuilder hostBuilder, Action<WindowsServiceLifetimeOptions> configure)

So these extensions can be used with the new model.

@halter73
Copy link
Member Author

My suggestion for these would be to add IServiceCollection versions of the extension methods. Both of these should work the same without the entire IHostBuilder.

The only exception is UseWindowsService not calling UseContentRoot(AppContext.BaseDirectory). AppContext.BaseDirectory is already the default content root for HostBuilder and HostApplicationBuilder even without any calls to ConfigureDefaults.

namespace Microsoft.Extensions.Hosting
{
    public static class SystemdServiceCollectionExtensions
    {
        public static IServiceCollection UseSystemd(this IServiceCollection services);
    }
    public static class WindowsServiceLifetimeServiceCollectionExtensions
    {
        public static IServiceCollection UseWindowsService(this IServiceCollection services);
        public static IServiceCollection UseWindowsService(this IServiceCollection services, Action<WindowsServiceLifetimeOptions> configure);
    }
}

Should I open a new issue with the API proposal or just put this back in the queue?

@eerhardt
Copy link
Member

I think either?

@halter73 halter73 added api-ready-for-review API is ready for review, it is NOT ready for implementation and removed api-approved API was approved in API review, it can be implemented labels Feb 25, 2022
@bartonjs
Copy link
Member

bartonjs commented Mar 15, 2022

Video

Instead of making new types for the feature parity extension methods, we think it's best to put them on their (slightly inappropriately named) existing types.

namespace Microsoft.Extensions.Hosting
{
     public static partial class Host
     {
        public static HostApplicationBuilder CreateApplicationBuilder();
        public static HostApplicationBuilder CreateApplicationBuilder(string[] args);
        public static IHostBuilder CreateDefaultBuilder();
        public static IHostBuilder CreateDefaultBuilder(string[] args);
    }
    public sealed class HostApplicationBuilder
    {
        public HostApplicationBuilder();
        public HostApplicationBuilder(string[] args);
        public HostApplicationBuilder(HostApplicationBuilderSettings settings);
        public ConfigurationManager Configuration { get; }
        public IHostEnvironment Environment { get; }
        public ILoggingBuilder Logging { get; }
        public IServiceCollection Services { get; }
        public IHost Build();
        public void ConfigureContainer<TBuilder>(IServiceProviderFactory<TBuilder> factory, Action<TBuilder>? configure = null) where TBuilder : notnull;
    }
    public sealed class HostApplicationBuilderSettings
    {
        public HostApplicationBuilderSettings();
        public string ApplicationName { get; set; }
        public string[] Args { get; set; }
        public string ContentRootPath { get; set; }
        public bool DisableDefaults { get; set; }
        public string EnvironmentName { get; set; }
        public ConfigurationManager Configuration { get; set; }
    }
    public partial static class SystemdHostBuilderExtensions
    {
        public static IServiceCollection UseSystemd(this IServiceCollection services);
    }
    public partial static class WindowsServiceLifetimeHostBuilderExtensions
    {
        public static IServiceCollection UseWindowsService(this IServiceCollection services);
        public static IServiceCollection UseWindowsService(this IServiceCollection services, Action<WindowsServiceLifetimeOptions> configure);
    }
}

@bartonjs bartonjs added api-approved API was approved in API review, it can be implemented and removed api-ready-for-review API is ready for review, it is NOT ready for implementation labels Mar 15, 2022
@WeihanLi
Copy link
Contributor

WeihanLi commented May 1, 2022

The ConfigureWebHost extensions also based on IHostBuilder,
https://github.com/dotnet/aspnetcore/blob/ac39742bf152a0d2980059289822e1d3526a880a/src/DefaultBuilder/src/GenericHostBuilderExtensions.cs#L32, the HostApplicationBuilder would not support for the WebHost?

@halter73
Copy link
Member Author

halter73 commented May 3, 2022

the HostApplicationBuilder would not support for the WebHost?

Correct. While it was nice to be able to add a web host to any arbitrary IHostBuilder, this isn't supported by HostApplicationBuilder. The recommendation is to use WebApplicationBuilder instead which is the inspiration for the HostApplicationBuilder type to begin with.

Is the some scenario that you have @WeihanLi where you'd want to use ConfigureWebHost on a HostApplicationBuilder but WebApplicationBuilder is insufficient?

@WeihanLi
Copy link
Contributor

WeihanLi commented May 4, 2022

@halter73 thanks for your reply, just to confirm, WebApplicationBuilder works well for me.

@nvmkpk
Copy link

nvmkpk commented May 13, 2022

Why not instead add a new type ConsoleApplication and ConsoleApplicationBuilder. WebApplication and WebApplicationBuilder (and any other application type anyone else might want to build for ex. MauiApplication, WpfAplication etc.) could just inherit from these new types.

@davidfowl
Copy link
Member

@nvmkpk What would that solve?

@nvmkpk
Copy link

nvmkpk commented May 13, 2022

@davidfowl It would provide a common base implementation for WebApplication like implementation. I implemented a library with similar code for my WPF application.

@davidfowl
Copy link
Member

Is that related to this issue?

@nvmkpk
Copy link

nvmkpk commented May 13, 2022

Yes, rather than making changes to HostBuilder, build the new types from scratch and have WebApplication and WebApplicationBuilder inherit from these.

@davidfowl
Copy link
Member

This issue is about configuration...

@nvmkpk
Copy link

nvmkpk commented May 16, 2022

Yes but that is to make it easier to use in WebApplicationBuilder right? My suggestion is to instead implement a new class as that seems easier to me. If you disagree, that is perfectly fine.

@halter73
Copy link
Member Author

halter73 commented May 20, 2022

I want to make a slight change to the approved UseWindowsService() and UseSystemd() APIs after starting work on the PR at #68580.

I want to rename these extension methods to AddWindowsService() and AddSystemd() for consistency with our other IServiceCollection extension methods. "Use" sticks out like a sore thumb when it's near other calls to builder.Services.AddWhatever(). We might not be 100% consistent with using the "Add" (and "Configure") prefix, but we're close. "Use" is far more commonly associated with middleware and to a lesser-extent IHostBuilder extension methods. Ultimately, we're just adding services like all the other IServiceCollection extension methods.

Here's the proposed diff from the already-approved API:

namespace Microsoft.Extensions.Hosting
{
    public static class SystemdServiceCollectionExtensions
    {
-        public static IServiceCollection UseSystemd(this IServiceCollection services);
+        public static IServiceCollection AddSystemd(this IServiceCollection services);
    }
    public static class WindowsServiceLifetimeServiceCollectionExtensions
    {
-        public static IServiceCollection UseWindowsService(this IServiceCollection services);
+        public static IServiceCollection AddWindowsService(this IServiceCollection services);
-        public static IServiceCollection UseWindowsService(this IServiceCollection services, Action<WindowsServiceLifetimeOptions> configure);
+        public static IServiceCollection AddWindowsService(this IServiceCollection services, Action<WindowsServiceLifetimeOptions> configure);
    }
}

Sample Usage

var builder = Host.CreateApplicationBuilder(args);

builder.Services.AddWindowsService();
builder.Services.AddHostedService<MyHostedService>();

builder.Build().Run();

@halter73 halter73 added blocking Marks issues that we want to fast track in order to unblock other important work api-ready-for-review API is ready for review, it is NOT ready for implementation and removed api-approved API was approved in API review, it can be implemented labels May 20, 2022
@davidfowl
Copy link
Member

Thank you @halter73

@bartonjs
Copy link
Member

The rename looks good as proposed.

namespace Microsoft.Extensions.Hosting
{
    public static class SystemdServiceCollectionExtensions
    {
-        public static IServiceCollection UseSystemd(this IServiceCollection services);
+        public static IServiceCollection AddSystemd(this IServiceCollection services);
    }
    public static class WindowsServiceLifetimeServiceCollectionExtensions
    {
-        public static IServiceCollection UseWindowsService(this IServiceCollection services);
+        public static IServiceCollection AddWindowsService(this IServiceCollection services);
-        public static IServiceCollection UseWindowsService(this IServiceCollection services, Action<WindowsServiceLifetimeOptions> configure);
+        public static IServiceCollection AddWindowsService(this IServiceCollection services, Action<WindowsServiceLifetimeOptions> configure);
    }
}

@bartonjs bartonjs added api-approved API was approved in API review, it can be implemented and removed blocking Marks issues that we want to fast track in order to unblock other important work api-ready-for-review API is ready for review, it is NOT ready for implementation labels May 26, 2022
@halter73
Copy link
Member Author

halter73 commented Jul 5, 2022

Thanks again for the help @eerhardt!

@halter73 halter73 closed this as completed Jul 5, 2022
@ghost ghost locked as resolved and limited conversation to collaborators Aug 4, 2022
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-Extensions-Hosting
Projects
None yet
Development

No branches or pull requests

9 participants