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

Improve async support on the startup path #24142

Open
davidfowl opened this issue Jul 21, 2020 · 20 comments
Open

Improve async support on the startup path #24142

davidfowl opened this issue Jul 21, 2020 · 20 comments
Assignees
Labels
affected-most This issue impacts most of the 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-major This label is used by an internal tool
Milestone

Comments

@davidfowl
Copy link
Member

davidfowl commented Jul 21, 2020

There are a few places where we lack async support that result in people making blocking call. These are using during application startup.

@davidfowl davidfowl added the Epic Groups multiple user stories. Can be grouped under a theme. label Jul 21, 2020
@davidfowl davidfowl changed the title Improve async support Improve async support on the startup path Jul 21, 2020
@BrennanConroy
Copy link
Member

We'll put in Next Sprint Planning for now. When are you expecting this work to be done, 5.0 or later?

@BrennanConroy BrennanConroy added this to the Next sprint planning milestone Jul 22, 2020
@ghost
Copy link

ghost commented Jul 22, 2020

Thanks for contacting us.
We're moving this issue to the Next sprint planning milestone for future evaluation / consideration. We will evaluate the request when we are planning the work for the next milestone. To learn more about what to expect next and how this issue will be handled you can read more about our triage process here.

@davidfowl
Copy link
Member Author

6.0

@ghost
Copy link

ghost commented Jul 23, 2020

We've moved this issue to the Backlog milestone. This means that it is not going to be worked on for the coming release. We will reassess the backlog following the current release and consider this item at that time. To learn more about our issue management process and to have better expectation regarding different types of issues you can read our Triage Process.

@Tratcher Tratcher added affected-few This issue impacts only small number of customers enhancement This issue represents an ask for new feature or an enhancement to an existing one severity-minor This label is used by an internal tool labels Oct 6, 2020 — with ASP.NET Core Issue Ranking
@ghost
Copy link

ghost commented Nov 4, 2020

Thanks for contacting us.
We're moving this issue to the Next sprint planning milestone for future evaluation / consideration. We will evaluate the request when we are planning the work for the next milestone. To learn more about what to expect next and how this issue will be handled you can read more about our triage process here.

@davidfowl davidfowl removed the affected-few This issue impacts only small number of customers label Mar 28, 2021
@davidfowl davidfowl added affected-most This issue impacts most of the customers severity-major This label is used by an internal tool Needs: Design This issue requires design work before implementating. and removed severity-minor This label is used by an internal tool labels Apr 2, 2021
@BrennanConroy
Copy link
Member

Please file some issues for the smaller work items.

@ghost
Copy link

ghost commented Apr 20, 2021

Thanks for contacting us.
We're moving this issue to the Next sprint planning milestone for future evaluation / consideration. Because it's not immediately obvious that this is a bug in our framework, we would like to keep this around to collect more feedback, which can later help us determine the impact of it. We will re-evaluate this issue, during our next planning meeting(s).
If we later determine, that the issue has no community involvement, or it's very rare and low-impact issue, we will close it - so that the team can focus on more important and high impact issues.
To learn more about what to expect next and how this issue will be handled you can read more about our triage process here.

@daiplusplus
Copy link

daiplusplus commented Jun 15, 2021

Surely it's better to solve this problem in Microsoft.Extensions.DependencyInjection instead of ASP.NET Core?

I've identified these scenarios that currently have no good solutions:

  • Async one-off application startup initialization.
    • Example: Loading program data from disk on startup via async filesystem/IO APIs prior to ConfigureServices.
  • Async service factories for singleton services.
  • Async service factories for transient and request-scoped services.
    • Example: injecting a service that needs to perform one-off initialization which involves (for example) async IO (otherwise it would have to perform lazy async init on every member invocation, which then means compromising the service's public API design (i.e. cannot use properties, every method must be async even if it's a trivial getter method, etc) all because of a tiny need to perform IO to initialize.

I note that approaches that you might think would work... don't work:

  • We cannot instantiate our own TStartup and pass values in to its constructor.
  • We cannot abuse IConfiguration because it only accepts String-properties, not arbitrary objects.
  • Since ASP.NET Core 3, we cannot use an early call to ConfigureServices to provide services for TStartup indirectly.
    • UPDATE: I just tried this now in an ASP.NET Core 3.1.10 project and it worked fine. That's weird considering all the people complaining about this being broken.

I think we should accept that Microsoft.Extensions.DependencyInjection needs to support Func<IServiceProvider,Task<T>> async service factories. Consider that the whole point of factories in general is to allow constructors to be simple by offloading complex initialization outside the constructor. Given it's unavoidable that some services will depend on IO, and that IO should ostensibly be async, there's no way around it - at least not one that doesn't involve compromising the design of services' public API surface.

I also suggest that support be added for eagerly-executed service factories for Singletons which would be invoked when services.BuildServiceProvider() (or rather: await services.BuildServiceProviderAsync()) is called. I'd like to see this feature specifically because I want more-or-less deterministic behaviour of my "heavy lifting" init code which I want to be sure runs before my ASP.NET Core process transitions to starting to handle incoming requests. I note that this approach means that ConfigureServices remains a non-async method.

Finally, being able to pass arbitrary objects and data into TStartup is essential. Having to resort to using static fields to pass data around into ConfigureServices feels wrong.


Regarding async service factories, on a technical basis, rather than having Task<TService> GetRequiredServiceAsync<TService>() on IServiceProvider, it could use ValueTuple to allow for concurrent async service resolution, example below.

Task<TService> GetRequiredServiceAsync<TService>()

Task<(TS1, TS2)> GetRequiredServicesAsync<TS1, TS2>()

Task<(TS1, TS2, TS3)> GetRequiredServicesAsync<TS1, TS2, TS3>()

Task<(TS1, TS2, TS3, TS4)> GetRequiredServicesAsync<TS1, TS2, TS3, TS4>()

Task<(TS1, TS2, TS3, TS4, TS5)> GetRequiredServicesAsync<TS1, TS2, TS3, TS4, TS5>()

// etc

So a contrived ConfigureServices could look like this (I recognize performing OIDC token refreshes inside an async factory is a bad example, especially given its propensity for failure, but it fits as an illustrative example):

void ConfigureServices( ServiceCollection services )
{
    services.AddScoped<IMyPreauthenticatedHttpClient>( asyncFactory: async sp => {
        ( IHttpContextAccessor hca, IHttpClientFactory hcf, IOidcClient oidcClient ) = await sp.GetRequiredServicesAsync<IHttpContextAccessor, IHttpClientFactory, IOidcClient>();
        String accessToken = await hca.HttpContext.GetTokenAsync("access_token");
        if( /* accessTokenIsExpired */ ) accessToken = await oidcClient.RefreshTokenAsync( hca.HttpContext );
        return new MyPreauthenticatedHttpClient( accessToken, hcf );
    } );
}

And for AddSingleton, I'd like to see a Boolean eagerFactory parameter which would cause implementationFactory or asyncFactory to be invoked and complete inside BuildServiceProviderAsync.


To summarize:

  • Perform async init in async Task Main and pass any object-graph into to TStartup or ConfigureServices without resorting to static state.
  • Guaranteed eager initialization of certain singleton services (async or not) prior to ASP.NET Core handling any incoming request.
  • Support for configuring async factories on ServiceCollection.
    • Async singleton service factories
    • Async scoped services factories
    • Async transient services factories

@davidfowl
Copy link
Member Author

davidfowl commented Jun 15, 2021

Perform async init in async Task Main and pass any object-graph into to TStartup or ConfigureServices without resorting to static state.

That was fixed in .NET 5 with this overload of UseStartup which lets you create the instance. Now you can use closures and ctor arguments again.

If you need services passed into Startup then make another DI container because the container isn't mutable and that isn't going to change.

Also the new hosting model removes some of the Program.cs/Startup.cs boilerplate so passing state in closures is possible and in a top level async main method. It makes the whole thing easier.

Guaranteed eager initialization of certain singleton services (async or not) prior to ASP.NET Core handling any incoming request.

dotnet/runtime#43149 go add your +1. It's not happening for .NET 6 though. As for adding the bool the AddSingleton. That's a problem because it needs to be agreed upon by the other DI authors but if you can get consensus we can move forward with that design or we'll have to pick another one.

Support for configuring async factories on ServiceCollection.

I don't know how we'd support any of this in reality. The problem with async factories is that you need async constructors to inject them into. It's viral, which means GetService would need to either block or throw. It also means that async factories can have both sync and async dependencies but sync factories can't depend on async things (or it'll be a pit of failure). This means you can't constructor inject an async service. In your example above, where does IMyPreauthenticatedHttpClient get resolved?

We'd also need GetServiceAsync (as you described above), but not the generic overloads, that could be an extension method you wrote. BuildServiceProviderAsync (as you also describe) and a new ServiceDescriptor that 3rd party container authors could implement.

Anyhow, thanks for the write up. Those are real concrete problems that are hard to solve.

PS: it would be interested to start a discussion on async DI and see what others think. I don't know how you'd solve the constructor problem but maybe we will find some interesting solutions.

EDIT:

In general, you'd want to split your IO graph from your non IO graph because they have very different performance characteristics. You want to be doing all of the impure stuff on the outside, and not have IO scattered within the construction of your dependency graph.

@daiplusplus
Copy link

daiplusplus commented Jun 15, 2021

@davidfowl (It's 3am, what are you doing up?!)

dotnet/runtime#43149 go add your +1.

Thanks - I've done that now. I did search around earlier but I didn't find this issue otherwise I would have done. (Can we agree GitHub search needs improvement?)

The problem with async factories is that you need async constructors to inject them into

I don't think that applies. The idea being that the IServiceProvider has logic to await any async dependencies first, before passing them into synchronous constructors - or anything with an async factory is registered as Task<T> instead of as T.

...the catch being that any consumer of GetService that requests a service with an async factory will (ideally) receive an exception and must call GetServiceAsync instead - however I note that well-designed service implementations won't be calling GetService directly, they should all be depending on constructor parameters. I'll concede my argument is very weak if actual real-world usage of GetService is higher than I expect (read: want) it to be.

(In my own code, I only ever call GetService when I need to hand-write an implementationFactory: sp => method, which is rare for me - so I'm assuming everyone else is like that)

It's viral, which means GetService would need to either block or throw

I would expect it to throw new InvalidOperationException("The requested service has an async factory. Call and await GetServiceAsync instead of GetService."). This wouldn't be a breaking change as it only impacts new code moving forward, as well as having limited-impact as the expectation is the end-application-developer is in control of the DI process - so if an external dependency requires a service without an async factory, but the host application unilaterally changed the registration to have an async factory then the application's developer can update their service registration to not use an async factory. Also, I note that when external dependencies do have their own second-order dependencies then they'll provide their own IServiceCollection extensions for registering their dependencies anyway - so things would only break if the host application decided to add their own async factories to override or replace the external library's.

Another possibility is that given that the IHost is very much aware of its environment, it could use safely use the thread-pool to run async factories without deadlocks and block the GetService call until everything resolves or a timeout.

In your example above, where does IMyPreauthenticatedHttpClient get resolved?

When I think about it, I think this concept could be implemented without any cooperation from Microsoft.Extensions.DependencyInjection by having the extension-methods proxy the async TService registrations for Task<TService>, and that combined with dynamically-generated types with constructors for Task<TService> could then be passed into ActivatorUtilities, then they'd have an AwaitAll method used by a hypothetical async ControllerFactory, and then the rest continued as normal.

Something like this:

ServiceCollectionExtensions:

public static IServiceCollection AddScoped<TService>( this IServiceCollection services, Func<IServiceProvider,Task<TService>> asyncFactory )
{
    return services.AddScoped<Task<TService>>( implementationFactory: asyncFactory );
}

ConfigureServices()

    services.AddScoped<IMyPreauthenticatedHttpClient>( asyncFactory: async sp => {
        ( IHttpContextAccessor hca, IHttpClientFactory hcf, IOidcClient oidcClient ) = await sp.GetRequiredServicesAsync<IHttpContextAccessor, IHttpClientFactory, IOidcClient>();
        String accessToken = await hca.HttpContext.GetTokenAsync("access_token");
        if( /* accessTokenIsExpired */ ) accessToken = await oidcClient.RefreshTokenAsync( hca.HttpContext );
        return new MyPreauthenticatedHttpClient( accessToken, hcf );
    } );

MyController.cs

public class MyController : Controller
{
    public MyController( IMyPreauthenticatedHttpClient client, MyDbContext dbContext )
    {
        // ...
    }
}

The ASP.NET Core request plumbing would then detect if the Controller's constructor has any dependencies with async-factories and if so await them all before returning the constructed Controller by using the dynamic type approach:

public async Task<ControllerBase> CreateControllerAsync( Type controllerType )
{
    if( UsesAnyAsyncRegisteredServices( controllerType ) )
    {
        Type taskHolderDynamicType = GetOrCreateCachedDynamicTypeFor( controllerType );
        IAsyncTaskHolder taskHolder = (IAsyncTaskHolder)ActivatorUtilities.CreateInstance( taskHolderDynamicType );
        return await taskHolder.AwaitServicesAndConstructControllerAsync();
    }
    else
    {
        // Same as right now.
    }
}

interface IAsyncTaskHolder
{
    Task<ControllerBase> AwaitServicesAndConstructControllerAsync();
}

Where, in my MyController's case, the dynamicly-defined type would be something like:

// This class created dynamically by ASP.NET for each concrete Controller class with constructor-injected dependencies with async factories.
public class MyControllerTaskHolder
{
    private readonlyTask<IMyPreauthenticatedHttpClient> clientTask;
    private readonly MyDbContext db;

    public MyControllerTaskHolder(
        Task<IMyPreauthenticatedHttpClient> client,
        MyDbContext db // <-- doesn't have an async factory, so accept it directly.
    )
    {
        this.clientTask = clientTask;
        this.db = db;
    }

    public async Task<ControllerBase> AwaitServicesAndConstructControllerAsync()
    {
        await Task.WhenAll( this.clientTask, /* any other tasks */ );
        return new MyController( this.clientTask.Result, this.db, etc )
    }
}

@soenneker
Copy link

I'm definitely looking forward to this. Essentially everything underlying the application startup has or is moving to async, and making the startup path that way would improve the fluidity. Things like initializing a Service Bus listener, or connecting to KeyVault can't even be done synchronously.

Hoping this gets in .NET 7

@davidfowl
Copy link
Member Author

davidfowl commented Feb 7, 2022

It's not happening in .NET 7. I'm not sure its ever going to happen actually. I haven't seen or thought of any feasible proposal as yet.

@loic-sharma
Copy link
Contributor

loic-sharma commented Feb 7, 2022

David mentioned that we need to inject async factories into constructors, thereby requiring async constructors. Why do we need to inject async factories?

Jehoel suggested making the IServiceProvider await the async factories and then passing the awaited service to sync constructors - would that approach not work?

@ghost
Copy link

ghost commented Feb 15, 2022

We've moved this issue to the Backlog milestone. This means that it is not going to be worked on for the coming release. We will reassess the backlog following the current release and consider this item at that time. To learn more about our issue management process and to have better expectation regarding different types of issues you can read our Triage Process.

@davidfowl
Copy link
Member Author

@loic-sharma yes, it would mean you could never use these types of services in places where you can already inject things today (namely constructors). You would be forced into the service locator pattern whenever you wanted to use one of these services. I think that defeats one of the purposes of DI.

That said, this is about async startup in ASP.NET Core, async DI is a part of it but not the only part.

@NickCraver
Copy link
Member

@davidfowl I've gotten many requests for this, increasing since the move to managed identity which is something that has to be fetched to pass into many services during startup, but unsure how much of that feedback makes its way all the way back to the team. I think the need for some of this support is escalating and would love to help justification here from the prod services that have pinged me directly if it helps.

Very specifically, it's Configure()/ConfigureServices() that's the most problematic, and where most people find themselves doing sync-over-async and usually in ways that could shave quite a bit off remote connections like Redis startup and unblocking the app sooner if they had a cleaner approach (not that they can't do it messily now, but a lot of pings haven't done so...so they end up with a wait for identity, then another blocking wait to start remote connections sub-optimally).

I'd be arguing not for GetServiceAsync(), only that even if ConfigureServices() was, as an entire unit, an internally-awaited .ConfigureServicesAsync() path like it is today but allowed and encouraged users down the right path of async-for-async-things inside it in their config, then at least it makes startup much easier to cleanly do correctly and optimally without needed to fully support all the downstream async service support.

Has some middle-ground like this to solve a lot of the pain for this area been considered? Or does that back us into a corner?

@davidfowl
Copy link
Member Author

davidfowl commented Mar 16, 2024

Move to the new pattern where the statup class is explicitly instantiated by user code and Configure and ConfigureServices are can be explicitly called which makes it easier to make them async.

@davidfowl
Copy link
Member Author

var builder = WebApplication.CreateBuilder(args);

var startup = new Startup(builder.Configuration);

await startup.ConfigureServicesAsync(builder.Services);

var app = builder.Build();

await startup.ConfigureAsync(app);

app.Run();

This doesn't solve the IStartupFilter problem, but it gives more control over how these methods are called.

@davidfowl
Copy link
Member Author

Those interested in async DI, I wrote a proposal here dotnet/runtime#65656

@mrudat
Copy link

mrudat commented May 8, 2024

I think that you want to be able to request both TService and Task<TService> for all services, regardless of how they're created/registered.

If your class doesn't do any async initialisation, you don't need to care. If you do, you can request a Task for everything (regardless of how it was created) and only await it when required.

That means when or if a dependency changes to async construction, your code doesn't need to change to take advantage of it.

For example, if you don't have any async initialisation in the second class, both classes have the same behaviour.

public class SyncConstructor(TService service) {}

public class AsyncConstructor{
    private AsyncConstructor(TService service) {}

    [AsyncConstructor]
    public static async Task<AsyncConstructor> Factory(Task<TService service>) {
        // async stuff here...
        return new AsyncConstructor(await service);
    }
}

Of course, it only makes sense to wait for potentially async objects.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
affected-most This issue impacts most of the 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-major This label is used by an internal tool
Development

No branches or pull requests