This repository has been archived by the owner. It is now read-only.

Added support for generic host based IWebHostBuilder #1580

Merged
merged 19 commits into from Nov 14, 2018

Conversation

Projects
None yet
@davidfowl
Copy link
Member

davidfowl commented Nov 10, 2018

  • This adds an implementation of IWebHostBuilder as a facade over the IHostBuilder.
    This removes the 2 container issue by executing the Startup.ConfigureServies and Startup.ConfigureContainer inline as part of building the IHostBuilder.
  • The implementation is highly compatible implementation since it exposes the same IWebHostBuilder interface.
    Existing extensions mostly work.
  • There are some caveats with this approach.
    • Injecting services into Startup is not extremely constrained to the
      services availble on HostBuilderContext. This includes the IHostingEnvironment
      and the IConfiguration.
    • IStartup is broken when using this pattern because it isn't composable.
    • 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)
    • Calling into IWebHostBuilder.Build will throw a NotSupportedException since
      this implementation is just a facade over the IHostBuilder.
    • TestServer isn't compatible with this implementation as yet.

Fixes #1544

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

  • - HostingContextContainsAppConfigurationDuringConfigureLogging => Hosting configuration flows to ConfigureLogging call backs application configuration does not (Won't fix)
  • - HostingContextContainsAppConfigurationDuringConfigureServices => Hosting configuration flows to ConfigureServices callbacks not application configuration does not (Won't fix)
  • - ExternalContainerInstanceCanBeUsedForEverything => Adding an IServiceProviderFactory as a service no longer works. Call IHostBuilder.UseServiceProviderFactory instead. (Won't fix)

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>();
                });
    }
}
Added support for generic host based IWebHostBuilder
- This adds an implementation of IWebHostBuilder as a facade over the IHostBuilder.
This removes the 2 container issue by executing the Startup.ConfigureServies and Startup.ConfigureContainer inline as part of building the IHostBuilder.
- The implementation is highly compatible implementation since it exposes the same IWebHostBuilder interface.
Existing extensions mostly work.
- There are some caveats with this approach.
    - Injecting services into Startup is not extremely constrained to the
    services availble on HostBuilderContext. This includes the IHostingEnvironment
    and the IConfiguration.
    - IStartup is broken when using this pattern because it isn't composable.
    - 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).
    - Calling into IWebHostBuilder.Build will throw a NotSupportedException since
    this implementation is just a facade over the IHostBuilder.

@davidfowl davidfowl requested review from glennc and Tratcher Nov 10, 2018

davidfowl added some commits Nov 10, 2018

@davidfowl

This comment has been minimized.

Copy link
Member

davidfowl commented Nov 11, 2018

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

This comment has been minimized.

Copy link
Contributor

jdom commented Nov 11, 2018

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

This comment has been minimized.

Copy link
Member

davidfowl commented Nov 11, 2018

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

This comment has been minimized.

Copy link

tillig commented Nov 11, 2018

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

@davidfowl

This comment has been minimized.

Copy link
Member

davidfowl commented Nov 11, 2018

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

davidfowl added some commits Nov 11, 2018

Unify the tests for the GenericWebHostBuilder and WebHostBuilder
- There's 3 failures but 2 are a known incompatiblity and the last one is a bug
Fix ordering issues with IHostingStartup
- Put callbacks in place first to run IHostingStartup modifications using a wrapper IWebHostBuilder (now we have 3 of them...).
Added light up interfaces for IWebHostBuilders
- Added ISupportsStartup and ISupportsDefaultServiceProvider that circumvents the default extension method logic and delegates to the type itself.
@oferns

This comment has been minimized.

Copy link

oferns commented Nov 11, 2018

@dotnetjunkie might be interested in this conversation

@seesharper

This comment has been minimized.

Copy link

seesharper commented Nov 11, 2018

@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

This comment has been minimized.

Copy link
Member

davidfowl commented Nov 11, 2018

@seesharper yes! That's exactly correct.


public void Dispose() => _host.Dispose();

public void Start() => _host.Start();

This comment has been minimized.

@rynowak

rynowak Nov 12, 2018

Member

Is this really right? Is the fact that this class is an implementation of IWebHost even significant since users can't call .Build()?

This comment has been minimized.

@rynowak

rynowak Nov 12, 2018

Member

I suppose this is here so you can resolve it as a service, but if you do - is it right to have these methods do what they do?

This comment has been minimized.

@psibernetic

psibernetic Nov 12, 2018

Why wouldn't webhost just call base on build?

This comment has been minimized.

@davidfowl

davidfowl Nov 12, 2018

Member

This only exists for testing so none of it matters.

internal class GenericWebHostApplicationLifetime : IApplicationLifetime
{
private readonly Microsoft.Extensions.Hosting.IApplicationLifetime _applicationLifetime;
public GenericWebHostApplicationLifetime(Microsoft.Extensions.Hosting.IApplicationLifetime applicationLifetime)

This comment has been minimized.

@rynowak

rynowak Nov 12, 2018

Member

You from a year ago is trolling you now 😆

This comment has been minimized.

@davidfowl
services.TryAddTransient<IServiceProviderFactory<IServiceCollection>, DefaultServiceProviderFactory>();

// Ensure object pooling is available everywhere.
services.TryAddSingleton<ObjectPoolProvider, DefaultObjectPoolProvider>();

This comment has been minimized.

@rynowak

rynowak Nov 12, 2018

Member

Is it worth rethinking some of these kinds of things? In retrospect the choice to make object pool a built-in is really strange.

This comment has been minimized.

@davidfowl

davidfowl Nov 12, 2018

Member

Agreed. Not something I want to change as part of this PR but defiantly something we should revisit.

This comment has been minimized.

@Tratcher

Tratcher Nov 12, 2018

Member

Now is a great time to drop it before anyone takes a dependency on it.

This comment has been minimized.

@davidfowl

davidfowl Nov 12, 2018

Member

No. I'll file an issue for removing things we don't like.

@tillig

This comment has been minimized.

Copy link

tillig commented Nov 13, 2018

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

This comment has been minimized.

Copy link

poke commented Nov 13, 2018

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

This comment has been minimized.

Copy link
Member

Tratcher commented Nov 13, 2018

@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

This comment has been minimized.

Copy link

poke commented Nov 14, 2018

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

This comment has been minimized.

Copy link
Member

Tratcher commented Nov 14, 2018

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

@davidfowl

This comment has been minimized.

Copy link
Member

davidfowl commented Nov 14, 2018

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

@PureKrome

This comment has been minimized.

Copy link

PureKrome commented Nov 14, 2018

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

This comment has been minimized.

Copy link
Member

davidfowl commented Nov 14, 2018

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

This comment has been minimized.

Copy link
Member

davidfowl commented Nov 14, 2018

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

@Tratcher
Copy link
Member

Tratcher left a comment

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

@davidfowl

This comment has been minimized.

Copy link
Member

davidfowl commented Nov 14, 2018

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

😍

@davidfowl davidfowl merged commit cfe9b26 into master Nov 14, 2018

4 checks passed

Hosting-ci #20181113.3 succeeded
Details
continuous-integration/appveyor/pr AppVeyor build succeeded
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
license/cla All CLA requirements met.
Details
@PureKrome

This comment has been minimized.

Copy link

PureKrome commented Nov 14, 2018

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

This comment has been minimized.

Copy link
Member

davidfowl commented Nov 14, 2018

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

This comment has been minimized.

Copy link

poke commented Nov 14, 2018

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

This comment has been minimized.

Copy link
Member

davidfowl commented Nov 14, 2018

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

This comment has been minimized.

Copy link
Member

davidfowl commented Nov 14, 2018

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

@poke

This comment has been minimized.

Copy link

poke commented Nov 14, 2018

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

This comment has been minimized.

Copy link
Member

davidfowl commented Nov 14, 2018

"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.

@Tratcher Tratcher deleted the davidfowl/webhost-builder-generic-host branch Nov 14, 2018

@davidfowl

This comment has been minimized.

Copy link
Member

davidfowl commented Dec 10, 2018

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

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.