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

Closed
Tratcher opened this issue Apr 12, 2019 · 55 comments
Closed

[Announcement] Generic Host restricts Startup constructor injection #9337

Tratcher opened this issue Apr 12, 2019 · 55 comments
Labels
breaking-change This issue / pr will introduce a breaking change, when resolved / merged.
Milestone

Comments

@Tratcher
Copy link
Member

Tratcher 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:
https://github.com/aspnet/AspNetCore/blob/5cb615fcbe8559e49042e93394008077e30454c0/src/Templating/src/Microsoft.DotNet.Web.ProjectTemplates/content/EmptyWeb-CSharp/Program.cs#L20-L22
3.0:
https://github.com/aspnet/AspNetCore/blob/b1ca2c1155da3920f0df5108b9fedbe82efaa11c/src/ProjectTemplates/Web.ProjectTemplates/content/EmptyWeb-CSharp/Program.cs#L19-L24

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

@Tratcher Tratcher added discussion breaking-change This issue / pr will introduce a breaking change, when resolved / merged. labels Apr 12, 2019
@Tratcher Tratcher added this to the Discussions milestone Apr 12, 2019
@springy76
Copy link

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
Copy link
Member Author

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
Copy link

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
Copy link
Member Author

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
Copy link

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
Copy link

springy76 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
Copy link
Member

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
Copy link

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

@Tratcher
Copy link
Member Author

Tratcher commented May 8, 2019

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

@NinoFloris
Copy link

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
Copy link

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
Copy link
Member Author

@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
Copy link

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
Copy link
Member Author

@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
Copy link

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
Copy link
Member Author

Tratcher 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
Copy link

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
Copy link

dustinlacewell 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
Copy link

wpbrown 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
Copy link

nblumhardt 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
Copy link

francoishill 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"));
    ...
}

@dazinator
Copy link

dazinator commented Oct 28, 2019

I'm not suggesting building the container twice either, I am suggesting passing the additional services to enable DI via an overload of UseStartup<>:

b.UseStartup<Startup>(hostInfo);

@davidfowl
Copy link
Member

That might be reasonable. To pass a separate collection or bag of values to Startup.

@springy76
Copy link

Yes please. These instances would then be available as singleton-scoped during entire lifetime?

@dazinator
Copy link

dazinator commented Oct 29, 2019

@springy76

These instances would then be available as singleton-scoped during entire lifetime?

I'm not convinced that this overload should automatically register these instances passed in this way for DI as singletons. You may not want them registered with DI at all, or you may want to use a different scope like transient etc. This method would just use them when activating the startup type in addition to the inbuilt instances (IHostEnvironment etc) that are already supported. If you wanted these objects to also be registered as singletons with DI you can do that fairly easily by registering them yourself, in addition to passing them in this manner. I'e using the existing mechanisms for configuring services (ConfigureServices method in Startup class, or within Host.ConfigureWebHostDefaults).

@alistairjevans
Copy link

Perhaps a simple solution here might be to allow the Startup class to be instantiated via a Lambda, so you can pass whatever you want to the constructor of your own Startup class? Something like:

// Get whatever logger you want
var myLogger = new SomeLogger();

// Create the host
var host = Host.CreateDefaultBuilder(args)
    .UseServiceProviderFactory(new AutofacServiceProviderFactory())
    .ConfigureWebHostDefaults(webHostBuilder =>
    {
      // Configure the startup, but with a lambda
      webHostBuilder
        .UseStartup((hostingEnvironment, configuration) => new Startup(hostingEnvironment, myLogger))
        .UseContentRoot(Directory.GetCurrentDirectory());
    })

Arguments to the lambda could be the things available to .NET at that time, i.e. hosting environment and configuration, then you add your own things.

@Tratcher
Copy link
Member Author

@alistairjevans at that point why not just use the IWebHostBuilder Configure and ConfigureServices APIs directly and capture what you need? Startup doesn't offer much beyond those.

@dazinator
Copy link

dazinator commented Oct 29, 2019

Some further background info about how I'd plan to use this feature

During web app startup I commonly find I need to repeat certain tasks.

For example:

  1. Configure a location where files can be persisted accross deployments.
  2. Configure DataProtection by persisting keys to a configured persistent storage location in a way that will also work in a web farm scenario or just a single server.
  3. Ensure a Logger is setup early, for logging errors prior to the container being built (they do happen and logs can be helpful)
  4. Lots of other things I consider plumbing for the majority of apps I work on.

My plan to stop from repeating myself in all apps, is to ship a nuget package that has an extension method for WebHostBuilder in it. Something like UseOpinionatedStartup<TStartup>()

This extension method would do all of the above opinionated plumbing, and then internally call webhostBuilder.UseStartup<TStartup>() to ensure the callers startup class is used as usual. Except now their startup class can focus less on all this plumbing and more the application specifics.

Now I can do all of this so far, already. The thing I cant currently do, is, as a consumer of this nuget package, when calling webhostBuilder.UseOpionatedStartup<MyStartup>() know it will do a bunch of stuff for me, but I have no way to access information about this opinionated startup process - such as the persistent content directory, the jwt signing key, the logger that was created early etc etc because there is no way to pass any additional data into MyStartup class. Therefore today, I am left using Statics, which kind of makes the Startup class activation process redundant. The extension method does register particular services (like IHostInfo) to expose this kind of additional information - but these services can only be accessed after the container is built which is too late as MyStartup needs to use this information to govern how services are registered into the container. So suggestion is to allow the extension method to do something like this:

var hostInfo = ConfigureHostOpinionated();
webHostBuilder.UseStartup<TStartup>(hostInfo);

Then in MyStartup class, I can have that injected into the constructor if I need it:

public MyStartup
{
   public MyStartup(IHostEnvironment hostEnvironment, IConfiguration config, IHostInfo hostInfo)
   {
        // Can now use extended info from host when configuring services.
        hostInfo.PersistentContentDirectory;
        hostInfo.JwtSigningKey;
        hostInfo.?? ; //etc etc
   }
}

This also explains why the Lambda idea doesn't help me in this case - The Extension method doesn't know how to construct the Startup class passed by the caller - and the caller is free to change their Startup class constructor. Therefore I need to activate the class - I could build my own activation logic of course, but I'd prefer if this useful feature was inbuilt for me to leverage, that way if asp.net core chooses to change the services that can be injected into startup classes by default, I wont have any logic to mimic.

@MV10
Copy link

MV10 commented Jan 12, 2020

@dazinator If I'm following you, it seems you could simply skip the use of Startup (which is just a convention, as mentioned earlier) and store the various pieces of shared information in the IHostBuilder.Properties collection under one or more pre-defined keys in your UseOpinionatedStartup implementations. That's pretty much what the Properties collection is for -- it isn't passed to Startup but it's basically the "bag of values" that davidfowl mentioned.

As for the logging issue, I'm starting to hear from people who are realizing there is a problem. We have a lot of headless jobs that rely solely on logging for progress and failure reporting. I've seen a few cobbled-together work-arounds. People need this so they will try to find an answer, which may be worse than the problem that was meant to be solved. Building early and breaking DI is not a solution. I'm trying to devise an in-house standard along the lines of caching messages until the builder runs. Exceptions actually run a new logger-only builder (which hopefully doesn't fail itself) merely to dump the cache and the exception ... but it feels like a hack, and of course, this lightweight pseudo-logger lacks the many important features of a real modern logger.

+1 for the observations that logging is a special-case / cross-cutting and if it needs special handling, so be it. This is non-trivial.

@MV10
Copy link

MV10 commented Jan 12, 2020

I spent the day cleaning up my implementation of reliable basic logging during Host Builder configuration. I still need to get it packaged for public use, but maybe somebody will find it useful.

https://github.com/MV10/GenericHostBuilderLogger

@MichaelPuckett2
Copy link

How do you pass command line args to Startup?

@Tratcher
Copy link
Member Author

@MichaelPuckett2 inject IConfiguration, that will contain your command line args if you provided them to the host builder.

@razzemans
Copy link

I also run into issues with this. We are setting a custom ISecureDataFormat in the CookieAuthenticationOptions and running into a production issue where decoding the data fails at random - I really like to pass a logger to our implementation of the ISecureDataFormat to see what is going wrong. In my opinion this should be trivial and I am disappointed that I don't have a way of doing that at the moment.

public void ConfigureServices(IServiceCollection services)
{
	services.AddAuthentication(o =>
	{
		o.DefaultScheme = CookieAuthenticationDefaults.AuthenticationScheme;
	})
	.AddCookie(o =>
	{
		o.TicketDataFormat = new CustomDataFormat(settings.Secret, /* Add logging? */);
	});
}

@Tratcher
Copy link
Member Author

Tratcher commented Feb 3, 2020

@razzemans does this help?

            services.AddAuthentication(options =>
            {
                options.DefaultScheme = CookieAuthenticationDefaults.AuthenticationScheme;
            }).AddCookie(CookieAuthenticationDefaults.AuthenticationScheme);

            services.AddTransient<IConfigureOptions<CookieAuthenticationOptions>>(sp =>
                new ConfigureNamedOptions<CookieAuthenticationOptions>(CookieAuthenticationDefaults.AuthenticationScheme, o =>
                {
                    o.TicketDataFormat = new CustomDataFormat("secret", sp.GetRequiredService<ILogger<CustomDataFormat>>());
                }));

https://docs.microsoft.com/en-us/aspnet/core/fundamentals/configuration/options?view=aspnetcore-3.1#use-di-services-to-configure-options

builder.Services.AddOptions<CookieAuthenticationOptions>(authenticationScheme).Validate(o => o.Cookie.Expiration == null, "Cookie.Expiration is ignored, use ExpireTimeSpan instead.");

https://github.com/dotnet/extensions/blob/cc317e6112479d36ba77a57e1eeea5d88a164a6c/src/Options/Options/src/OptionsServiceCollectionExtensions.cs#L213
https://github.com/dotnet/extensions/blob/cc317e6112479d36ba77a57e1eeea5d88a164a6c/src/Options/Options/src/OptionsBuilder.cs#L73-L74

We're missing some overloads of AddCookie or Configure that would make this scenario easier.

.AddCookie(CookieAuthenticationDefaults.AuthenticationScheme, (o, serviceProvider) =>
{
        o.TicketDataFormat = new CustomDataFormat("secret", sp.GetRequiredService<ILogger<CustomDataFormat>>());
});

.AddCookie<ILogger<CustomDataFormat>>(CookieAuthenticationDefaults.AuthenticationScheme, (o, logger) =>
{
        o.TicketDataFormat = new CustomDataFormat("secret", logger);
});

services.Configure<CookieAuthenticationOptions, ILogger<CustomDataFormat>>(CookieAuthenticationDefaults.AuthenticationScheme, o =>
{
       o.TicketDataFormat = new CustomDataFormat("secret", sp.GetRequiredService<ILogger<CustomDataFormat>>());
});

Let me know which of these look useful and I'll file an issue. [Edit] Filed #18772

@razzemans
Copy link

Thanks a lot @Tratcher - that indeed works.

I think I would prefer your first proposal - it is also consistent with the factory methods overloads of the IServiceCollection interface.

@IanKemp
Copy link

IanKemp commented Mar 12, 2020

I'm with the other people in this thread calling for a special-case exemption for logging: it's absolutely necessary. The fact that this apparently wasn't considered at all during Core 3.0 dev is absolutely bizarre, in my opinion.

I understand the rationale behind not doing it, and the hoops that @MV10 jumped through to make a usable workaround that doesn't break everything shows how difficult it would be to do without the "fake" HostBuilder from 2.x, but this really feels like a case of the programmers being forced to serve the code, not the opposite.

Please strongly consider adding this capability back as soon as possible. Fobbing people off with half-a**ed workarounds, or requiring them to install third-party NuGet packages to get this basic functionality back, is not an acceptable solution in any way shape or form. Usability needs to trump ideology here.

@dazinator
Copy link

dazinator commented Mar 12, 2020

To echo @IanKemp, and add some info, there is nothing stopping us right now from initialising logging early (creating an ILoggerProvider instance, be that console, serilog etc) immediately in program main, and using that to create an ILogger instance and log with right away in Program.cs. This is great to catch and log host startup errors- just remember to dispose of ILoggerProvider in a finally block so it has a chance to flush any messages before the app terminates. This approach works in conjunction with AddLogging() which happens sometime later - you can call AddProvider() to add your instance of your ILoggerProvider that you initialised early in Program main so that the same instance is available for DI.

All good so far. A few issues remain for the framework to address though in my view:

  1. It should actively encourage your to initialise a logger as the first thing you do. If the framework knows about this initial ILoggerProvider instance (which it should imho) then it could also automatically include it in the Providers list where AddLogging is called (it already includes other loggers by default) and it could use this logger itself when doing any host level startup stuff, including allowing this ILogger to be injected into any startup class.

  2. When using startup classes, the host doesnt allow you to activate them supplying your own additional constructor parameters over and above what it knows how to inject natively. If it did then we could optionally supply an ILogger instance or anything else that we initialised early in program.cs, and our logic in startup.cs could now have the benefit of being able to log. I work around this currently by referencing the ILogger from a static field e.g Program.Logger but this hurts testability of startup.cs.

To address that part we'd just need the ability to supply additional constructor parameter values for activating startup classes, so we could do:

HostBuilder.UseStartup<TStartup>(loggerInstance)

@davidfowl
Copy link
Member

It should actively encourage your to initialise a logger as the first thing you do.

Sounds like you're suggesting the templates change to do this.

If the framework knows about this initial ILoggerProvider instance (which it should imho) then it could also automatically include it in the Providers list where AddLogging is called (it already includes other loggers by default) and it could use this logger itself when doing any host level startup stuff, including allowing this ILogger to be injected into any startup class.

This requires some more thought but it might work if we do something hacky like try to detect an existing ILoggerProvider. There are still 2 DI containers in this universe so for example, you'd need to make sure to configure options for logging in the right place as the streams don't cross.

When using startup classes, the host doesnt allow you to activate them supplying your own additional constructor parameters over and above what it knows how to inject natively. If it did then we could optionally supply an ILogger instance or anything else that we initialised early in program.cs, and our logic in startup.cs could now have the benefit of being able to log. I work around this currently by referencing the ILogger from a static field e.g Program.Logger but this hurts testability of startup.cs.

This sounds reasonable and it's something we do today for middleware (see UseMiddleware).

@danzinator Can you file 2 separate issues for these items? No promises of course but having 2 concrete proposals would help the discussion.

@dazinator
Copy link

@davidfowl ok, have created #19807 and #19809

@ghost
Copy link

ghost commented Dec 2, 2020

Thank you for contacting us. Due to a lack of activity on this discussion issue we're closing it in an effort to keep our backlog clean. If you believe there is a concern related to the ASP.NET Core framework, which hasn't been addressed yet, please file a new issue.

This issue will be locked after 30 more days of inactivity. If you still wish to discuss this subject after then, please create a new issue!

@ghost ghost closed this as completed Dec 2, 2020
@ghost ghost locked as resolved and limited conversation to collaborators Jan 1, 2021
This issue was closed.
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
breaking-change This issue / pr will introduce a breaking change, when resolved / merged.
Projects
None yet
Development

No branches or pull requests