aspnet / Hosting

[Archived] Code for hosting and starting up an ASP.NET Core application. Project moved to https://github.com/aspnet/Extensions and https://github.com/aspnet/AspNetCore
Apache License 2.0
552 stars 312 forks source link

Added support for generic host based IWebHostBuilder #1580

Closed davidfowl closed 5 years ago

davidfowl commented 5 years ago

Fixes #1544

There are 3 incompatibilities found in running the existing test suite:

This is what we're aiming for with the 3.0 template:

using System.Threading.Tasks;
using Microsoft.AspNetCore.Hosting;
using Microsoft.Extensions.Hosting;

namespace SampleApplication
{
    public class Program
    {
        public static async Task Main(string[] args)
        {
            await CreateHostBuilder(args).Build().RunAsync();
        }

        public static IHostBuilder CreateHostBuilder(string[] args) =>
            Host.CreateDefaultBuilder(args)
                .ConfigureWebHostDefaults(builder =>
                {
                    builder.UseStartup<Startup>();
                });
    }
}
davidfowl commented 5 years ago

This might be the way we recommend frameworks plug into the generic host. A single top level method with a builder specific to that framework. I'm thinking that Orleans could potentially use the same approach with the SiloBuilder.

cc @ReubenBond @jdom

jdom commented 5 years ago

I'm glad you went with this approach, as it aligns with what we were thinking for Orleans too. The sub builder approach makes the target of the configuration very clear when you are hosting more than 1 app framework in the same host.

davidfowl commented 5 years ago

cc @tillig

Just an FYI that this breaks the pattern where you return an IServiceProvider from Startup.ConfigureServices. The preferred pattern will be an IServiceProviderFactory. That makes things compose since the web application configuration isn't the only thing existing. This will require a migration story when moving apps from 2.x to 3.x. Luckily, we're not breaking any of the WebHostBuilder behavior so it can be smooth 😄 .

tillig commented 5 years ago

Thanks for the heads-up. I'll make sure our corresponding docs and examples get updated!

davidfowl commented 5 years ago

Pinging other DI container authors (feel free to spread the word if I missed people) cc @dadhi @seesharper @ipjohnson @khellang @ENikS

oferns commented 5 years ago

@dotnetjunkie might be interested in this conversation

seesharper commented 5 years ago

@davidfowl Thanks for letting us know up front about changes like this.

Correct me if I am wrong, but this is what I can gather from this

davidfowl commented 5 years ago

@seesharper yes! That's exactly correct.

javiercn commented 5 years ago
  • The IStartupConfigureServicesFilter and IStartupConfigureContainer The before and after filters added in 2.1 are also broken because there's a single container (it could maybe be fixed by downcasting and doing something specific on the GenericHostBuilder instance) (@javiercn)

I think I know how we can fix this easily. We can use the new properties dictionary in IHostBuilderContext (should be an abstract class) to tag the filter instances in there and then when ConfigureServices is invoked on the host builder, we simply retrieve the filters from the hostbuildercontext and apply them.

tillig commented 5 years ago

Just an FYI that this breaks the pattern where you return an IServiceProvider from Startup.ConfigureServices. The preferred pattern will be an IServiceProviderFactory.

Currently the only way to build the Autofac container and keep a reference to it if you need to do that (e.g., for later service location using the Autofac container directly) is to return the IServiceProvider from Startup.ConfigureServices. We've also seen some interesting use cases where people want to use a single container and host multiple application types (like a self-hosted ASP.NET Core app alongside... uh... something else? Not sure what). That's led to requests for things like the ability to create a pseudo-container in a child lifetime scope.

+-------------------------------------------------------+
| root container: shared registrations                  |
| and singletons                                        |
|                                                       |
| +------------------------+ +------------------------+ |
| | child scope: root      | | child scope: root      | |
| | for ASP.NET Core self- | | for background service | |
| | hosted app             | | or something (?)       | |
| +------------------------+ +------------------------+ |
+-------------------------------------------------------+

Having the ability to return the IServiceProvider means the dev could provide a pre-created scope to the ConfigureServices method and return a custom-built IServiceProvider.

I gather that in order to do something like this in the new system folks will need a custom IServiceProviderFactory or figure out a workaround.

poke commented 5 years ago

Is there any way we can bring back the functionality that allows us to inject an logger into the Startup? It’s very common for applications to log certain things during startup, at a time when the DI container has not been built.

I understand that with this implementation, there is only hard-coded support for IHostingEnvironment and IConfiguration but I think ILogger<T> or ILoggerFactory would be another of the few types that actually make sense to get injected there.

I’ve already looked into the HostBuilder source for this but unfortunately, logging is just set up through DI so it will be difficult to get a proper logger factory out of it. But I think logging would be a really valuable addition to those explicit types that are supported in the startup.

Tratcher commented 5 years ago

@poke I don't see how it's possible to bring logging back, it fundamentally depends on DI and config. It's worth noting that logging was already broken (multiple singletons) and one of the reasons we dropped the second container.

poke commented 5 years ago

Yeah, I understand that it’s very difficult and not possible in the current design, but I really think that it’s something that is expected to work. So if we don’t end up having a good approach for that, I am sure people will make up a way to achieve this, for example by creating a new (second) logger factory from within their startup. Just like people built the service provider within the ConfigureServices method when they needed some random service to configure stuff before it was properly communicated (and supported by utility methods) how to do that properly.

Tratcher commented 5 years ago

At least logging can still be injected into Configure, it's only ConfigureServices that's missing it.

davidfowl commented 5 years ago

Logging can't really be done because logger providers are themselves in the DI container.

PureKrome commented 5 years ago

So if we are logging when we ConfigureServices, where a method (in here) takes an ILogger, then we would need to create a new (Second) logger factory (as @poke said) ?

e.g.

var builder = new HostBuilder()
    .ConfigureServices((hostContext, services) =>
    {
        services.AddLogging(configure => configure.AddSerilog(dispose: true));

        var myConfigSettings = services.AddConfigurationSettings(Configuration);

        services.AddHostedService<MyService>();
        services.AddMyDb(myConfigSettings.DatabaseSettings, --here should be an ILogger --)
    });

await builder.RunConsoleAsync();
davidfowl commented 5 years ago

So if we are logging when we ConfigureServices, where a method (in here) takes an ILogger, then we would need to create a new (Second) logger factory (as @poke said) ?

If you're logging today in ConfigureServices then you're in this torn state where you're using another instance of the ILoggerFactory that's configured. What that can lead to is several background threads running for each of the logger factories (for example the console logger and blob storage logger).

Sure. you can create another logger factory that is separate in there but where would you be logging to?

davidfowl commented 5 years ago

@tillig Yes, you'd need a custom factory to do that, but you can capture the instance by using the Configure method.

davidfowl commented 5 years ago

Update https://github.com/aspnet/Hosting/blob/master/samples/GenericWebHost/ and then I think you're good for

😍

PureKrome commented 5 years ago

So if we are logging when we ConfigureServices, where a method (in here) takes an ILogger, then we would need to create a new (Second) logger factory (as @poke said) ?

If you're logging today in ConfigureServices then you're in this torn state where you're using another instance of the ILoggerFactory that's configured. What that can lead to is several background threads running for each of the logger factories (for example the console logger and blob storage logger).

yep. but can we dispose of this factory (and therefore any bg threads) once we leave the scope? Then there could be issues of flushing, etc ?

Sure. you can create another logger factory that is separate in there but where would you be logging to?

Console / some 3rd party service

in the example above i have:

services.AddLogging(configure => configure.AddSerilog(dispose: true));

so the logging sinks are read-in/defined -prior- to the HostBuilder getting created. So logging has been added/configured for serilog ... and then I was hoping to use this seril-logger as a MEL ILogger. Happy that there's no DI in this bit .. but possible to expose/use currently existing one?

davidfowl commented 5 years ago

yep. but can we dispose of this factory (and therefore any bg threads) once we leave the scope? Then there could be issues of flushing, etc ?

Today, sure, but disposing things you didn't create is an anti pattern. Today the 2 containers hang around for the lifetime of the application.

I would do that outside of the HostBuilder completely. Make another container just for logging add the providers and resolve the factory, then used that for logging and dispose it when configure services is over.

poke commented 5 years ago

Logging can't really be done because logger providers are themselves in the DI container.

I know but I think there still has to be some way to do it. Even if you don’t necessarily need to log stuff out from the ConfigureServices method, people will still want to do it (there’s a relatively popular SO question on this that proves the desire).

And I really don’t want people to end up doing things like this:

public void ConfigureServices(IServiceCollection services)
{
    var provider = services.BuildServiceProvider();
    var logger = provider.GetService<ILogger<Startup>>();

    logger.LogInformation("Adding MVC");
    services.AddMvc();
}

But that’s totally going to happen if we don’t have an actual recommendation on what to do instead.

You mentioned on Slack that you are considering a root logger as a singleton, and I think that would be totally fine. Just allow some sane way to get a logger inside of there, so we can direct people to that solution.

davidfowl commented 5 years ago

That will be the way to do it regardless, it will require another container. It should not be the same service provider that you use the build up the application though. We'll discuss it more.

davidfowl commented 5 years ago

PS that sample code you wrote in the answer makes 2 independent logger systems already. It's just implicit...

poke commented 5 years ago

Yeah, but since there already is a separate service provider at web host level by default, it just works. It even uses the same configuration and setup even if it’s technically a separate instance. And that would be my expectation with 3.0 too: That there would be a default and easy way that just works, even if this means that the framework will create a separate logger for me.

davidfowl commented 5 years ago

"Just works" There's been numerous unsolvable bugs around the fact that we have 2 containers (sometimes 3!). Most of our workarounds have been to not log anything until after the application container is wired up. We want to be out of that world.

davidfowl commented 5 years ago

cc @ENikS I can't remember if I mentioned you on this.