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

Add IAsyncStartup and IAsyncStartupFilter to support async startup classes and filters #5897

Closed
analogrelay opened this issue May 25, 2017 · 28 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 severity-minor This label is used by an internal tool
Milestone

Comments

@analogrelay
Copy link
Contributor

We should introduce a new set of interfaces: IAsyncStartup and IAsyncStartupFilter with the following API surface:

    public interface IAsyncStartup
    {
        IServiceProvider ConfigureServices(IServiceCollection services);

        Task ConfigureAsync(IApplicationBuilder app);
    }

    public interface IAsyncStartupFilter
    {
        Func<IApplicationBuilder, Task> Configure(Func<IApplicationBuilder, Task> next);
    }

Startup classes will now support a Task ConfigureAsync(IApplicationBuilder app) method, which is mutually exclusive with void Configure(IApplicationBuilder app). It shall be an error to have both methods on a startup class.

IAsyncStartup and IAsyncStartupFilter will be the primitives used by the entire Hosting pipeline. Adapters will exist in Hosting to allow the continued use of IStartup and IStartupFilter in the public API surface (to reduce breaking changes).

@analogrelay
Copy link
Contributor Author

/cc @muratg

@Drawaes
Copy link
Contributor

Drawaes commented May 25, 2017

Please do! I could use this now

@davidfowl
Copy link
Member

Don't change it, just add a new interface.

@analogrelay
Copy link
Contributor Author

That's basically what I meant, updating to clarify.

@analogrelay analogrelay changed the title Make IStartup and IStartupFilter async Add IAsyncStartup and IAsyncStartupFilter to support async startup classes and filters May 31, 2017
@khellang
Copy link
Member

Why does it have to be an error to have both? Couldn't you just have async be prioritized over sync? I.e. resolve IAsyncStartup first, then if It's not registered, resolve IStartup etc. StartupBase could implement both and have the async overload call the sync overload.

@davidfowl
Copy link
Member

I think async should win over sync

@analogrelay
Copy link
Contributor Author

analogrelay commented Jun 1, 2017

We certainly could, it just seems like it's unnecessarily ambiguous. Wouldn't you have to explicitly register both to get in that scenario? All our helpers should be updated to register just the one (IAsyncStartup) with the necessary adaptors.

Either way, I'm fine, as long as the behavior is clear.

@davidfowl
Copy link
Member

@anurse you may implement both on the same object.

@analogrelay
Copy link
Contributor Author

I suppose. I just still think that's a pretty unclear situation, since you'll have code that we'll never call ourselves. Anyway, prioritizing async is the right approach if we're going to allow both.

@Tratcher
Copy link
Member

@davidfowl The call stack for this doesn't make sense. Startup is instantiated and run from WebHostBuilder.Build(), not WebHost.StartAsync(), so Build() would have to become async for this.

@analogrelay
Copy link
Contributor Author

Yeah. Making Build async is out of scope for 2.0 I think, it's way too late. What about reopening #1085 and moving IHostedService initialization to before Server initialization?

@Tratcher
Copy link
Member

@muratg discussed this with @davidfowl offline, this should move to Backlog to revisit in 3.0.

@grahamehorner
Copy link
Contributor

please try to be consistent with async naming conventions StartAsync() StopAsync() on Hosting IWebHost would suggest that interface naming should be IStartUpAsync and IStartupFilterAsync ? thoughts

@davidfowl
Copy link
Member

I don't think that makes it any more consistent than IAsyncStartupFilter. The 2 things are completely unrelated.

@Tratcher
Copy link
Member

Tratcher commented Jul 9, 2017

@effyteva
Copy link

effyteva commented Sep 2, 2018

Any updates on this one? It's stuck for more than a year now..

@stijnherreman
Copy link

@effyteva see https://github.com/aspnet/Hosting/issues/1088#issuecomment-311741002, this issue will be revisited for the 3.0 release.

@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 enhancement This issue represents an ask for new feature or an enhancement to an existing one labels Dec 18, 2018
@NinoFloris
Copy link

Will this actually be revisited for 3.0?

@analogrelay
Copy link
Contributor Author

Unfortunately no, we had other higher priority work that means this is not scheduled for 3.0.

@davidfowl
Copy link
Member

davidfowl commented Apr 26, 2019

I spent a could of minutes looking at this and I don't think it;s possible to implement without blocking. We've basically deprecated IStartup in 3.0 so adding IAsyncStartup could work (or just supporting it without the interface) but the problem is that you cannot run sync filters around an async startup without blocking (sync over async). It would have to be this way until everyone moves over to async filters. Even worse is that once you enter first IStartupFilter, you've lost all asynchrony.

@gulbanana
Copy link

this is going to be a problem for blazor if it continues to use the same Startup mechanism. all i/o has to be async in blazor, and you can't just start a task to do it either - so right now there's no way to, say, initialise a service which depends on runtime-loaded data.

@MV10
Copy link

MV10 commented Sep 14, 2019

Is there a reason ConfigureServicesAsync would be problematic?

Similar to what @gulbanana notes, there are a lot of scenarios where that would be handy. For example, I inherited several systems that use .GetAwaiter().GetResult() to read config values from Azure Key Vault, Blob Storage, and a custom distributed cache mechanism. I've seen similar async database calls in other projects that retrieve auth config and so on. I suspect the guidance is probably "don't do that" but the fact is, people do in the real world. I wouldn't argue for doing-it-wrong on that basis alone, but my question is whether there is anything about ConfigureServices that means it must remain synchronous.

More generally, I understand why this change hasn't been made yet, but on the other hand, as people start getting the "async whenever possible" message, I'm seeing more and more .GetAwaiter().GetResult() out there. (At work where I do a lot of code reviews and give arch advice, I'm explaining how this is risky at an alarmingly increasing rate, usually after finding it in code and checking whether the dev understands what "sync-over-async" really means.)

I'm of the opinion that re-factoring Program.cs and Startup.cs for async isn't that much of a chore for the average ASP.NET app. Tear off the band-aid! 5.0 is a major release, breaking changes are the name of the game.

@damianh
Copy link

damianh commented Jul 20, 2020

Any progress on this? For 5.0 maybe?

I have several use cases for loading config async (i.e. AWS Secrets Manager / Azure Key Vault) and GetAwaiter().GetResult() is... yuck.

@davidfowl
Copy link
Member

There's no progress for 5.0 no, we don't know how to do this without blocking or breaking changes. It might be possible to run filters in 2 stages that never overlap.

@davidfowl
Copy link
Member

Hmm, that might actually work now that I've typed it out...

@davidfowl
Copy link
Member

Ah nope, the problem is the underlying call to Configure itself would need to be async OR sync. They can't interleave or overlap because of the calling pattern. One needs to replace the other.

@iSeiryu
Copy link

iSeiryu commented Aug 14, 2020

Say yes to breaking changes! Async all the way!
We had to deal with more complex changes in our projects when we had to migrate from .Net Framework to Core and from Core 2.0 to Core 3.1.
This one is more trivial and very much desired.
One article on https://docs.microsoft.com would clarify everything and people would circle it around very quickly.

@Tratcher Tratcher added affected-medium This issue impacts approximately half of our customers severity-minor This label is used by an internal tool labels Nov 6, 2020 — with ASP.NET Core Issue Ranking
@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 severity-minor This label is used by an internal tool
Projects
None yet
Development

No branches or pull requests