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

[Announcement] Generic Host restricts Startup constructor injection #9337

Open
Tratcher opened this issue Apr 12, 2019 · 32 comments

Comments

@Tratcher
Copy link
Member

commented Apr 12, 2019

TLDR: The only types the generic Host supports for Startup constructor injection are IHostEnvironment, IWebHostEnvironment, and IConfiguration. Applications using WebHost are unaffected.

In 3.0 we've re-platformed the web stack onto the generic host library. You can see the change in Program.cs in the templates:

2.x:

public static IWebHostBuilder CreateWebHostBuilder(string[] args) =>
WebHost.CreateDefaultBuilder(args)
.UseStartup<Startup>();

3.0:
public static IHostBuilder CreateHostBuilder(string[] args) =>
Host.CreateDefaultBuilder(args)
.ConfigureWebHostDefaults(webBuilder =>
{
webBuilder.UseStartup<Startup>();
});

One key behavior change here is that Host only uses one dependency injection container to build the application, as opposed to WebHost that used one for the host and one for the app. As a result the Startup constructor no longer supports custom service injection, only IHostEnvironment, IWebHostEnvironment, and IConfiguration can be injected. This change was made to avoid DI issues such as duplicate singleton services getting created.

Mitigations:

Inject services into Startup.Configure:
public void Configure(IApplicationBuilder app, IOptions<MyOptions> options)

[We'll add more based on requests for specific scenarios.]

@springy76

This comment has been minimized.

Copy link

commented Apr 16, 2019

Your mitigation does not work for me: Startup.Configure just doesn't have the information I need, because thats why I'm passing a complex object to the constructor of the Startup class.

In my app the entire asp-net-core block itself is just a little part of the entire app. It inherits outer services by the use of nested Autofac lifetime scopes and a bunch of delegates are being collected in earlier stages which configure aspnetcore services and routes -- all of that getting passed to the Startup class.

@Tratcher

This comment has been minimized.

Copy link
Member Author

commented Apr 16, 2019

Constructor injected services were only needed for use in Startup.ConfigureServices. Can you give specific examples of what you're doing in ConfigureServices that consumes services? It's often possible to set up delayed actions that only resolve services when your service is resolved.

@springy76

This comment has been minimized.

Copy link

commented Apr 16, 2019

It's often possible to set up delayed actions that only resolve services when your service is resolved

While this helps in many cases, here it does not, because aspnetcore is just one of many (optional) services in the app:

  • The app initially only processed file based jobs. Years ago Owin has been added to provide some adhoc API surface.
  • There is one big Autofac DI container which also contains services from (optional) plugins
  • one of the services (similar to BackgroundWorkers) pulls a class AspNetCoreInitializer from DI which itself pulls ILifetimeScope from Autofac DI
  • Startup then uses public IServiceProvider ConfigureServices(IServiceCollection services) to add additional dynamic services, using structures from AspNetCoreInitializer instance above, and finally uses the ILifetimeScope (from AspNetCoreInitializer again) to build a new nested Autofac lifetimescope (which inherits all outer services) which then gets returned
  • Startup.Configure then uses even more data from AspNetCoreInitializer to setup (plugin provided) SignalR hubs, WebDav routes, etc.

Having this written now, I wonder if .UseStartup<Startup>(); is a requirement or if everything the Startup class does can also be just done against I(Web)HostBuilder?

@BrennanConroy BrennanConroy changed the title [Annoucement] Generic Host restricts Startup constructor injection [Announcement] Generic Host restricts Startup constructor injection Apr 16, 2019
@Tratcher

This comment has been minimized.

Copy link
Member Author

commented Apr 16, 2019

Having this written now, I wonder if .UseStartup(); is a requirement or if everything the Startup class does can also be just done against I(Web)HostBuilder?

Indeed, Startup is not a required construct, it's a holdover from the OWIN days when there might not have been a Program.Main. It can be still useful for organizing code. Are you able to do everything you need to with I(Web)HostBuilder?

@springy76

This comment has been minimized.

Copy link

commented Apr 16, 2019

Since the AspNetCoreInitializer class I was speaking of already contains the code which calls new WebHostBuilder() this will be fine. Thank you for clarification.

@springy76

This comment has been minimized.

Copy link

commented Apr 17, 2019

Hmm. In my Startup class I have this method:

public IServiceProvider ConfigureServices(IServiceCollection services)

which returns the DI container to use. How would I do this with IWebHostBuilder?

@davidfowl

This comment has been minimized.

Copy link
Member

commented Apr 17, 2019

That does not work either. It doesn't compose with other methods on the IHostBuilder. There's a single DI container now that gets build after the application is fully composed.

You can now plug in a DI container using the IServiceProviderFactory:

public static IHostBuilder CreateHostBuilder(string[] args) =>
    Host.CreateDefaultBuilder(args)
        .ConfigureWebHostDefaults(webBuilder =>
        {
            webBuilder.UseStartup<Startup>();
        })
        .UseServiceProviderFactory(new AutofacServiceProviderFactory());
@NinoFloris

This comment has been minimized.

Copy link

commented May 8, 2019

And what about WebHostBuilderContext why isn't that injectable as its constituent parts are?

@Tratcher

This comment has been minimized.

Copy link
Member Author

commented May 8, 2019

@NinoFloris why would you want the WebHostBuilderContext? As you say, all of its fields are available individually.

@NinoFloris

This comment has been minimized.

Copy link

commented May 8, 2019

No reason in particular, separate is fine. I don't know, it may have felt a bit inconsistent as WebHostBuilder ConfigureServices does pass it.

@grecosoft

This comment has been minimized.

Copy link

commented Jun 19, 2019

Using the generic host builder, how do you log? Within the ConfigureServices method how do you write a log based on the configured logging? How can you reference ILogger or ILoggerFactory? Logging is important and should be easily accessible after the ConfigureLogging method is called.

@Tratcher

This comment has been minimized.

Copy link
Member Author

commented Jun 19, 2019

@grecosoft logging is built on dependency injection, and ConfigureLogging is just a wrapper for ConfigureServices. It's not possible to log until after all of the services have been registered and the container has been built.

@grecosoft

This comment has been minimized.

Copy link

commented Jun 19, 2019

Then how would you write logs. For example, if you wanted to write a log as to which service type was register? I understand you can access after the container is created. But how do you write logs as the application is being built?

@Tratcher

This comment has been minimized.

Copy link
Member Author

commented Jun 19, 2019

@grecosoft it's not supported in this model, or at least not until after ConfigureSerivices. Startup.Configure is one of the earliest phases where DI services like logging are available.

@grecosoft

This comment has been minimized.

Copy link

commented Jun 19, 2019

Ok. Seems to be limiting. Just seems that logging is so important that it should be created up front and allowed to be used anywhere. Is it possible to create a logger to be used during configuration?

@Tratcher

This comment has been minimized.

Copy link
Member Author

commented Jun 19, 2019

No, the logger depends on configuration too.

What your asking for requires a logger that is configured directly in code and doesn't use DI, Config, Options, etc.. That kind of logger is possible, but of very limited usefulness.

@grecosoft

This comment has been minimized.

Copy link

commented Jun 20, 2019

Tratcher - I understand. Not an issue just need to make a small refactor to our code. Thanks for the answering my questions and for such a fast response.

@dustinlacewell

This comment has been minimized.

Copy link

commented Jul 7, 2019

This is a real disappointment. By using the WebHost container, I was able to register a number of classes as "Binding Services" or "Startup Services" via attributes. Then I could use IEnumerable<IStartupBinder> and IEnumerable<IStartupService> in ConfigureServices and Configure respectively and my startup looked like this:

    public class Startup {

        private ILogger<Startup> _logger { get; set; }
        private IEnumerable<IStartupBinder> _binders { get; }


        public Startup(IConfiguration config, ILogger<Startup> logger, IEnumerable<IStartupBinder> binders) {
            _logger = logger;
            _binders = binders;
        }

        public void ConfigureServices(IServiceCollection services) {
            foreach (var startupBinder in _binders) {
                _logger.LogInformation($"Starting binder: {startupBinder.GetType().Name}");
                startupBinder.Bind(services);
            }
        }

        public void Configure(IApplicationBuilder app, IEnumerable<IStartupService> startupServices) {
            foreach (var startupService in startupServices) {
                _logger.LogInformation($"Starting service: {startupService.GetType().Name}");
                startupService.Startup(app);
            }
        }

    }

Which our team found to be a very clean design. We had a chuckle at the response that logging is simply "not supported in this model". Please reconsider whether this is the ideal way forward.

We certainly were not confused by the WebHost having a separate container. Maybe documentation was the answer instead of this backwards-incompatible change...

@wpbrown

This comment has been minimized.

Copy link

commented Jul 24, 2019

Just saying don't log until Configure feels wrong. I was previously injecting IOptions and ILoggers in to ConfigureServices that were known to be already fully configured during the WebHost build process. This worked well. FWIW the docs for 3.0 Startup still say you can inject a LoggerFactory.

@nblumhardt

This comment has been minimized.

Copy link

commented Aug 17, 2019

@wpbrown if you use Serilog, you can initialize it before building the host, and use it directly until it's wired into the Microsoft.Extensions.Logging subsystem during start-up.

This does mean using configuration directly from the files or environment, of course, since the configuration subsystem won't have been initialized, yet; e.g. WriteTo.File(path: Environment.GetEnvironmentVariable("MY_LOG_FILE_PATH")). Works for me. HTH!

@francoishill

This comment has been minimized.

Copy link

commented Sep 29, 2019

Workaround for a few Startup logs. This should not be used to create a logger that lives longer than the Startup Configuration.

USE AT OWN RISK!!!

using (var scope = services.BuildServiceProvider().CreateScope())
{
    var logger = scope.ServiceProvider.GetRequiredService<ILogger<Startup>>();
    logger.LogInformation("Application is starting up");
}

Important notes:

  • Ensure you call AddApplicationInsightsTelemetry before this
  • If you register any ITelemetryInitializer's and want them to fire before logging, ensure to register them beforehand as well

You can also create a method to make its usage cleaner:

private static void WriteLog(IServiceCollection services, Action<ILogger> action)
{
    using (var scope = services.BuildServiceProvider().CreateScope())
    {
        var logger = scope.ServiceProvider.GetRequiredService<ILogger<Startup>>();
        action(logger);
    }
}

public void ConfigureServices(IServiceCollection services)
{
    ...
    WriteLog(services, logger => logger.LogInformation("Application starting up"));
    ...
}
@Tratcher

This comment has been minimized.

Copy link
Member Author

commented Sep 29, 2019

Calling BuildServiceProvider is explicitly NOT recommended, it re-creates the exact problem we were trying to solve. Building the container multiple times means you end up with multiple instances of objects that were supposed to be singletons like the logging infrastructure. It can cause numerous problems in your app. Rebuilding the container for every log statement is even worse. @francoishill please redact that post, that advice is dangerous/harmful.

@francoishill

This comment has been minimized.

Copy link

commented Sep 30, 2019

Calling BuildServiceProvider is explicitly NOT recommended, it re-creates the exact problem we were trying to solve. Building the container multiple times means you end up with multiple instances of objects that were supposed to be singletons like the logging infrastructure. It can cause numerous problems in your app. Rebuilding the container for every log statement is even worse. @francoishill please redact that post, that advice is dangerous/harmful.

@Tratcher I hear your argument. This advice is not to be used to continually write logs for the lifetime of the application or inside some loop. It should be used for a few startup logs. I will edit my post accordingly.

There is, however, no other solution that seems to work. Do you have any other suggestions that would work to get the Startup logs into App Insights? Other suggestions did not work with <TargetFramework>netcoreapp2.2</TargetFramework> and <PackageReference Include="Microsoft.ApplicationInsights.AspNetCore" Version="2.8.0" />. None of the logs in Startup reach App Insights.

@cheesi

This comment has been minimized.

Copy link

commented Oct 1, 2019

I'm also missing the logging in the startup class. Kind of disappointing... Is there really no practicable in-build solution?

@wpbrown

This comment has been minimized.

Copy link

commented Oct 1, 2019

I was originally unsure what to do about this change. It forced me to rethink my approach, and I actually like this new model better. I chose to create a bare minimum "bootstrapping" ILogger (example), that is just used until the application is built. In my case it just logs everything to the Console or Windows Event Log and is not effected by end-user configuration (which could be considered a good thing for diagnosing application startup issues). If folks agree with this concept it may be worth suggesting in the docs.

@francoishill if you want to take the bootstrap logger concept above and also work with ApplicationInsights you can just call AddApplicationInsights on the ILoggingBuilder when you create your factory. App Insights normally filters to Warning level by default. If you want that on your logger too you can add the rule:

new LoggerFilterRule("Microsoft.Extensions.Logging.ApplicationInsights.ApplicationInsightsLoggerProvider", null, LogLevel.Warning, null)));
@francoishill

This comment has been minimized.

Copy link

commented Oct 2, 2019

Very nice @wpbrown, this approach works well and feels much more "native". The only gotcha being the InstrumentationKey is not automatically obtained. This means you need to get it from the config, for example with var instrumentationKey = _configuration.GetValue<string>("ApplicationInsights:InstrumentationKey");. Below is my approach in Startup to achieve this.

private readonly bool _isDevelopment;
private readonly IConfiguration _configuration;
private readonly ILogger _logger;

public Startup(IConfiguration configuration, IWebHostEnvironment env)
{
    _configuration = configuration;
    _isDevelopment = env.IsDevelopment();

    _logger = GetEarlyInitializationLogger();
}

private ILogger GetEarlyInitializationLogger()
{
    var instrumentationKey = _configuration.GetValue<string>("ApplicationInsights:InstrumentationKey");
    using var loggerFactory = LoggerFactory.Create(builder =>
    {
        builder.AddConsole();
        builder.AddApplicationInsights(instrumentationKey);
    });

    return loggerFactory.CreateLogger("Initialization");
}

// This method gets called by the runtime. Use this method to add services to the container.
public void ConfigureServices(IServiceCollection services)
{
    _logger.LogInformation("Application starting up");
}
@alistairjevans

This comment has been minimized.

Copy link

commented Oct 4, 2019

Just to add to the set of workarounds; if you use NLog then it's really easy to get a normal ILogger instance in the Startup class which you can use as you did before:

public class Startup
{
  public Startup(IConfiguration configuration)
  {
    // Get the factory for ILogger instances.
    var nlogLoggerProvider = new NLogLoggerProvider();

    // Create an ILogger.
    Logger = nlogLoggerProvider.CreateLogger(typeof(Startup).FullName);
  }
  
  public void ConfigureServices(IServiceCollection services)
  {
    Logger.LogInformation("Registering Services");
    // And the rest
  }
}

Some additional details are in my post about it: https://alistairevans.co.uk/2019/10/04/asp-net-core-3-0-logging-in-the-startup-class-with-nlog/

@Kaelum

This comment has been minimized.

Copy link

commented Oct 8, 2019

@Tratcher I'm sorry, but none of the options that you have presented, are acceptable. Logging must be available the moment Main() is started, and should be carried through to the end of the application. If this means that logging that needs to be handled differently from everything else, then so be it.

@alistairjevans there is a one liner that can be used for NLog anywhere within your application:

Logger = LogManager.GetCurrentClassLogger();

Until this is fixed properly in IHost, the above line is the only true work-around. I can't believe logging during StartUp is so screwed up.

@alistairjevans

This comment has been minimized.

Copy link

commented Oct 8, 2019

@Kaelum, the reason I didn't use GetCurrentClassLogger is because it returns an NLogger log abstraction, whereas I wanted to get hold of the proper .NET Core ILogger type. That way I can pass it to other objects that I might want to DI later, without leaking NLog into the rest of the app.

@Kaelum

This comment has been minimized.

Copy link

commented Oct 8, 2019

@alistairjevans don't forget to implement IDisposable, as the NLogLoggerProvider object needs to be disposed of. If you later replace Logger with the value from the Configure() method, you'll need to dispose of it there as well. Plus, this only works if the StartUp class is disposed of, which I haven't looked into the code to see if it would be. That's a little too convoluted for my taste.

Unless you are going to pass the NLogLoggerProvider object around, I'm not sure what you mean by "I can pass it to other objects". The logger will be tied to the object type that you created it under, unless you are saying that you're going to create additional ILogger objects for each class that you pass it to? Passing a single ILogger to other objects would cause NLog rules confusion issues.

@davidfowl

This comment has been minimized.

Copy link
Member

commented Oct 9, 2019

Until this is fixed properly in IHost, the above line is the only true work-around. I can't believe logging during StartUp is so screwed up.

You can do something like this:

using Microsoft.AspNetCore.Hosting;
using Microsoft.Extensions.DependencyInjection;
using Microsoft.Extensions.Hosting;
using Microsoft.Extensions.Logging;

namespace WebApplication356
{
    public class Program
    {
        public static void Main(string[] args)
        {
            using var loggerFactory = LoggerFactory.Create(builder =>
            {
                builder.AddConsole();
                builder.AddEventSourceLogger();
            });

            CreateHostBuilder(args).ConfigureServices(services => services.AddSingleton(loggerFactory)).Build().Run();
        }

        public static IHostBuilder CreateHostBuilder(string[] args) =>
            Host.CreateDefaultBuilder(args)
                .ConfigureWebHostDefaults(webBuilder =>
                {
                    webBuilder.UseStartup<Startup>();
                });
    }
}

There are some downsides though:

  • Logger providers added via DI won't work (since you took control)
  • You'll need to replicate this in your setup if you want to match host defaults. This gets messy since you need configuration to exist.
@Kaelum

This comment has been minimized.

Copy link

commented Oct 10, 2019

@davidfowl I think that the entire idea of creating the LoggerFactory within the HostBuilder is doomed for failure. Instead, the logger should be completely defined prior to the HostBuilder, and then used by the HostBuilder as it was created. This is why NLog and log4net allow you to configure them via a separate configuration file, and does not require you to even share the application configuration. They do support using the application configuration file, but it isn't recommended for this specific reason.

LoggerFactory should support the use of standalone configuration files, including the standalone files that NLog and log4net use. This makes significantly more sense than what we currently have. There is no reason that I can see as to why we are forced to use the application configuration for our logging configuration. They should be completely separate if we need them to be.

We're also moving away from JSON configuration files and back to XML configuration files, solely due to the reason that XML files transform better than JSON files do. It also doesn't make sense to wrestle with JSON file sudo transformations, when XML transformations are so easy to work with when using SlowCheetah.

Anywho, hopefully some day logging will stand alone.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
You can’t perform that action at this time.