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

Support an OnApplicationStart[Async] method or similar pattern #5890

Closed
divega opened this issue Sep 23, 2015 · 34 comments
Closed

Support an OnApplicationStart[Async] method or similar pattern #5890

divega opened this issue Sep 23, 2015 · 34 comments
Labels
affected-medium This issue impacts approximately half of our customers area-hosting Includes Hosting area-networking Includes servers, yarp, json patch, bedrock, websockets, http client factory, and http abstractions enhancement This issue represents an ask for new feature or an enhancement to an existing one Needs: Design This issue requires design work before implementating. severity-minor This label is used by an internal tool
Milestone

Comments

@divega
Copy link
Contributor

divega commented Sep 23, 2015

Applications often need to run code once at application startup per web server, e.g. to setup or initialize some external resource to be used by the application. There are two main issues with doing so in Startup.Configure():

  1. Lifetime of services can be incorrect at this stage, e.g. services that are registered as scoped will not be disposed correctly and will remain in memory
  2. It cannot execute asynchronously

One general idea that has been discussed is to have a middleware that will run this code on first request but use a lock to make sure it is only done once.

The main example where this have come up is dotnet/efcore#3070 "Pattern for seeding database with EF7 and ASP.NET 5". Note that we cannot recommend a pattern like this in general because it is not safe unless you know there is only one web server, but for many customers this can be enough. Note also that MusicStore currently uses a workaround.

Another possibly related scenario is described at dotnet/efcore#3070 (comment). Although it is not about code that needs to run at application start, it is about how to get services to be activated with the right lifetime on code that doesn't execute as part of a request:

Another problematic case is when you use a Timer. During the callback there have no scope. So the used instances are always the same, because you have to use the ServiceProvider of the application, so always the same context. What would be the best way to use DI with a Timer? The Timer sould perhaps create a scope during the callback and pass it in parameter? Use IServiceScopeFactory in the callback?

cc @Eilon

@divega divega changed the title Support OnApplicationStart[Async] method Support an OnApplicationStart[Async] method or similar pattern Sep 23, 2015
@Eilon
Copy link
Member

Eilon commented Sep 23, 2015

I think we had a bug on this a million years ago but who knows where that bug is...

I've always liked the middleware approach to this. It's kind of like the IIS warmup module.

@kevinchalet
Copy link
Contributor

I think we had a bug on this a million years ago but who knows where that bug is...

Just there, sir: aspnet/Hosting#29 😄

@Eilon
Copy link
Member

Eilon commented Sep 24, 2015

Yeah that's just about a million years ago! 😄

@Eilon
Copy link
Member

Eilon commented Sep 24, 2015

@Tratcher and recent thoughts on this?

@Tragetaschen
Copy link
Contributor

What about ApplicationStarted?

@Eilon
Copy link
Member

Eilon commented Sep 24, 2015

@Tragetaschen the scenario is an application warmup scenario, such as hydrating a cache, etc. So the requirement is to have some sort of "event" that is blocking and async. Not sure that cancellation token fits the bill.

@Tratcher
Copy link
Member

I think this really belongs in Startup.Configure.

  1. If there's an issue with scoped services in Configure then we should fix it. Making a scope for Configure shouldn't be a big deal.
  2. Async at startup is meaningless if you need the app to block until you're done anyways.

The middleware approach is not good for app initialization because it is only triggered by the first request, and that request needs to block all other requests while it does it's work. When it's done that middleware stays in the pipeline and runs on every request.

@Eilon
Copy link
Member

Eilon commented Sep 24, 2015

I don't know that the scopes can really be "fixed" because there's nothing actually wrong with it. The problem is that the app's "warmup" code requires those services, but is not running within the desired scope. The idea of a middleware (though perhaps it's not the right solution) is that the middleware would obviously run in the right scope (request scope).

I agree that actually being async is meaningless because it's blocking, but it allows the app developer to write non-ugly code (avoid calling Wait or .Result on everything).

As far as running on every request, well, the cost would presumably be negligible...

@gongdo
Copy link

gongdo commented Nov 27, 2015

Any updates about this?
Is this a recommendation for async-startup(or warm-up)?

@Eilon I agree that the cost of initialization blocking would be negligible, however It doesn't seem to be nice if I should write a blocking middleware for one-time initialization.
I'd like to choose Wait or Result rather than blocking middleware.

@muratg
Copy link
Contributor

muratg commented Jan 15, 2016

@Tratcher @Eilon Are we doing this or backlog?

@Eilon
Copy link
Member

Eilon commented Jan 15, 2016

No new features for now. Backlog.

@pauldotknopf
Copy link
Contributor

Is this still visible and on the backlog? I notice the tag says "rc2".

@davidfowl
Copy link
Member

No, there's no plan to do this.

@DenisDollfus
Copy link

DenisDollfus commented Apr 17, 2017

Our server sends a bunch of http requests during startup to get all kind of configuration, so this feature definitely makes sense (though I have no preference on the API shape specifically).

@chrispaynter
Copy link

This would be very helpful, especially when using platforms such as Contentful where in my case a lot of configuration lives. I need it at Startup but it takes an async network request to fetch it.

@Cyberboss
Copy link

Cyberboss commented Feb 4, 2018

I have a (albiet hacky) workaround for this using IApplicationLifetime.

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

namespace TGWebhooks.Core
{
    static class ApplicationBuilderExtensions
    {
		public static void UseAsyncInitialization<TInitializer>(this IApplicationBuilder app, Func<TInitializer, CancellationToken, Task> asyncInitializer)
		{
			var applicationLifetime = app.ApplicationServices.GetRequiredService<IApplicationLifetime>();
			Task initializationTask = null;
			applicationLifetime.ApplicationStarted.Register(() =>
			{
				var toInitialize = app.ApplicationServices.GetRequiredService<TInitializer>();
				initializationTask = asyncInitializer(toInitialize, applicationLifetime.ApplicationStopping);
			});

			app.Use(async (context, next) =>
			{
				await initializationTask;
				await next.Invoke();
			});
		}
	}
}

They must be the first handlers in the pipeline, TInitializer must be a singleton service, and block on the first request until finished. But it allows you to write code like this:

public void Configure(IApplicationBuilder app, IHostingEnvironment env)
{
	app.UseAsyncInitialization<IPluginManager>((pluginManager, cancellationToken) => pluginManager.Initialize(cancellationToken));

	if (env.IsDevelopment())
                app.UseDeveloperExceptionPage();

        app.UseMvc();
}

Update

I made it more flexible. (Cyberboss/AspNetCore.AsyncInitializer)(Nuget)

@giggio
Copy link
Contributor

giggio commented Oct 26, 2018

The problem on async startup is still pending. Would you consider it for v3?

@dotnetjunkie
Copy link

I don't think there is any practical benefit in allowing start-up to be async. I like to refer to a discussion about this I had with Stephen Cleary. My main point is:

Typically, there is no real benefit of doing start-up initialization asynchronously. There is no practical performance benefit, because at start-up time, there will only be a single thread running anyway (although we might parallelize it, but that obviously doesn’t require async). Also note that although some application types might deadlock on doing synch-over-async, in the Composition Root we know exactly which application type we are using and whether or not this will be a problem or not. A Composition Root is application-specific. In other words, when we have initialization in our Composition Root, there is typically no benefit of doing start-up initialization asynchronously.

Although some application types might deadlock while blocking async calls, this is not the case in ASP.NET Core. There would therefore not be any benefit in adding all sorts of asynchronous start-up abstractions.

@Cyberboss
Copy link

Isn't this solved with IHostedService now?

@Drawaes
Copy link
Contributor

Drawaes commented Oct 26, 2018

Two things

  1. Using aspnet as a webserver inside some other application (it's small enough to allow this) would be useful if you could start it async.

  2. It supports a cleaner model. We constantly tell users to never use result or wait and then say... Oh yeah but do it in startup which confuses the issue. The messaging should be never use wait or result and a single way of doing it. It's more a problem of saying "it's okay here" in this case.

@giggio
Copy link
Contributor

giggio commented Oct 27, 2018

I agree with both of @Drawaes points. My main concern is number 2. There is definitely a message problem here, exactly as he states.

If you decide to not do it it would be a good idea to publish docs bringing up some best practices on how to do this correctly. I still have nightmares with deadlocks from .Result or .Wait. When I saw that there was no way to do this asynchronously the first thought that came to mind was uh-oh! 😱

@pauldotknopf
Copy link
Contributor

Given that Program.Main supports async/await, I hope we can now let it flourish throughout the IWebHostBuilder/IHostBuilder internals completely.

It is easy to call sync code from async, but not vice-versa. Let's support the path of least resistance.

There may be some hidden details in the core libs that make this a significant effort to implement, but when it comes to the user code (implementations), having void Function(){} vs Task Function() { return Task.CompletedTask; } is minutiae. We don't need to have philosophical discussions about how this impacts user code, since it doesn't force already sync code to do anything special.

@CoskunSunali
Copy link
Contributor

Are there any known workarounds to this?

@matteocontrini
Copy link

@CoskunSunali I've been using AspNetCore.AsyncInitialization and it suits my needs. There's a blog post from the author that explains it

@Eilon
Copy link
Member

Eilon commented Dec 11, 2018

On one of my apps it was suggested to use an async Program.Main like so:

https://github.com/Eilon/Hubbup/blob/master/src/Hubbup.Web/Program.cs#L12-L24

This is from @Tratcher , so credit to him on this.

@CoskunSunali
Copy link
Contributor

@matteocontrini, @Eilon Thank you for the replies.

Both of the links show how to call async methods within main but I was wondering how to get the Startup class's Configure and ConfigureServices to be async.

I have plugins that need to register services during Configure but the plugins are loaded asynchronously.

So I am trying to achieve something similar to:

public class Startup {
    public async Task Configure(IServiceCollection services) {
        var plugins = await LoadPlugins();

        foreach (var plugin in plugins) {
            await plugin.ConfigureAsync(services);
        }
    }
}

That being said, I am aware of being able to run an async method synchronously but not sure it that breaks the further (inner?) asynchronous calls.

The workaround turns to be:

public class Startup {
    public void Configure(IServiceCollection services) {
        Task.WaitAll(Task.Run(async () => await ConfigurePluginServicesAsync(services)));
    }

    private async Task ConfigurePluginServicesAsync(IServiceCollection services) {
        var plugins = await LoadPlugins();

        foreach (var plugin in plugins) {
            await plugin.ConfigureAsync(services);
        }
    }
}

I would be glad if you can provide any insights.

@Tratcher
Copy link
Member

@CoskunSunali async Startup is not supported, and blocking is highly discouraged.

@abatishchev
Copy link

abatishchev commented Dec 11, 2018

Just Task.Run(() => ConfigurePluginServicesAsync(services)).Wait() would also work and that's what everyone uses today, to my knowledge. And what this issue is all about. Even if async startup makes no sense methodologically.

@davidfowl
Copy link
Member

That code that does a Task.Run followed by .Wait doesn’t make much sense. There’s no fear of deadlocking in ASP.NET Core applications so the additional Task.Run is kinda pointless.

We should look at async startup but today you can use an IHostedService to do you async initialization

@abatishchev
Copy link

The point is to be on the safe side of the equation, to always use same code on the sync-to-async boundary. Diversification of approaches in this case leads to mistakes. Any overhead is insignificant as the code runs just once.

@davidfowl
Copy link
Member

I think it gives a far sense of security. It’s usually presented as “the right way to make async code synchronous”. I’ve seen all of the ways and there are all different tradeoffs. If I saw this code in an application I would guess it was copy pasta and the developer didn’t actually understand what was going on.

@aspnet-hello aspnet-hello transferred this issue from aspnet/Hosting Dec 18, 2018
@aspnet-hello aspnet-hello added this to the Backlog milestone Dec 18, 2018
@aspnet-hello aspnet-hello added area-hosting Needs: Design This issue requires design work before implementating. labels Dec 18, 2018
@manigandham
Copy link

manigandham commented Dec 28, 2018

Might not be necessary now with the current DI system and async Program.Main. The WebHost is just a class that you build and run. You can do anything you want after the instantiation but before running the host, with access to registered services, scopes, correct disposal and async/await.

public static class Program
{
    public static async Task Main(string[] args)
    {
        using (var host = CreateWebHostBuilder().Build())
        {
            // get a service from the DI container and use it
            await host.Services.GetService<SomeService>().Start();
            
            // get and start multiple services at the same time
            await Task.WhenAll(
                host.Services.GetService<SomeService>().Start1(),
                host.Services.GetService<SomeOtherService>().Start2()
            );
            
            // use a scope if you need scoped services
            var sp = host.Services.GetService<IServiceScope>();
            using (var scope = sp.ServiceProvider.CreateScope())
            {
                scope.ServiceProvider.GetService<SomeScopedService>().RunSomething();
            }
            
            // start the actual webhost and run it
            await host.RunAsync();
            
            // webhost also has other methods for more control
            // this is basically what's inside RunAsync above
            await host.StartAsync();
            await host.WaitForShutdownAsync();
            await host.StopAsync();

            // all services implementing IDisposable will be disposed
            // when the WebHost is disposed and in reverse order from instantiation 
            // so you can control the dependencies
        }
    }
}

You can wrap this with extension methods or a subclass to make it cleaner. We use this code with several apps that have startup requirements for config, messaging, caching, etc. that need to be available before the app can serve requests, while also shutting down those services properly after requests are stopped.

@johan-v-r
Copy link

Adding another use case for my requirements is to fetch config used to register services:

private async Task RegisterHttpClientsAsync(IServiceCollection services, Uri configUri)
{
	using var configClient = new HttpClient { BaseAddress = configUri };
	var httpClients = await configClient.GetFromJsonAsync<IEnumerable<KeyValuePair<string, Uri>>>("foo");
			
	foreach (var client in httpClients)
	{
		services.AddHttpClient(client.Key, c => { c.BaseAddress = client.Value; });
	}
}

This can only be done before CreateWebHostBuilder().Build() to my knowledge..?
Also the HttpClient doesn't have "sync" functions anymore. Somewhere something's gotta give.

@davidfowl
Copy link
Member

Closing in favor of #24142

@ghost ghost locked as resolved and limited conversation to collaborators Apr 27, 2021
@amcasey amcasey added the area-hosting Includes Hosting label Jun 1, 2023
@amcasey amcasey added area-networking Includes servers, yarp, json patch, bedrock, websockets, http client factory, and http abstractions and removed area-runtime labels Aug 24, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
affected-medium This issue impacts approximately half of our customers area-hosting Includes Hosting area-networking Includes servers, yarp, json patch, bedrock, websockets, http client factory, and http abstractions enhancement This issue represents an ask for new feature or an enhancement to an existing one Needs: Design This issue requires design work before implementating. severity-minor This label is used by an internal tool
Projects
None yet
Development

No branches or pull requests