Add analyzer to detect async void usage

davidfowl commented 3 years ago

There are a couple of places where async void can be used by mistake in ASP.NET applications (and minimal APIs) and we should flag this and error with an analyzer:

[ ] Startup.Configure/ConfigureServices
[ ] MVC Controller Action methods
[ ] Razor Page handlers (https://stackoverflow.com/questions/67794528/cannot-access-a-disposed-object-object-name-iserviceprovider-error-in-aspnet)
[ ] MVC filters (the non IAsync*Filters)
[ ] SignalR Hub Methods
[ ] Minimal APIs returning void

We should flag all of these areas with a single analyzer and add more as we see fit.

ghost commented 3 years ago

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.

Youssef1313 commented 3 years ago

@davidfowl I'm interested to work on this. As far as I understand, a class that implements non-async filter with async-methods should be reported by the analyzer. Is that correct? Are there any other cases? Thanks.

JunTaoLuo commented 3 years ago

Sounds like we are unclear where/how this analyzer would ship cc @pranavkm @davidfowl we should discuss this before we can make progress on this analyzer.

davidfowl commented 3 years ago

@Youssef1313 are you still interested?

Youssef1313 commented 3 years ago

@davidfowl Yup! If possible, I need some more information since my knowledge to ASP.NET Core isn't that great. This is what I understand:

[ ] Startup.Configure/ConfigureServices

If a class name that starts with Startup (e.g, Startup, StartupDevelopment, etc.) contains an async void Configure or ConfigureServices method, then produce a diagnostic with a fixer to turn it to async Task.
[ ] MVC Controller Action methods

If any method in a class that derives from Controller contains an async void method, then produce a diagnostic with a fixer to turn it to async Task.
[ ] Razor Page handlers

Is the link beside this point in the issue was supposed to belong to the previous point? Otherwise, I need some distinction between this point and the previous one?
[ ] MVC filters (the non IAsync*Filters)

Take IActionFilter as an example, if a class that implements it has an async OnActionExecuting or OnActionExecuted, then produce a diagnostic with no codefix since the user should reason about that.

BrennanConroy commented 3 years ago

Hub methods could also be included here.

davidfowl commented 3 years ago

Added

ghost commented 3 years ago

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 commented 3 years ago

cc @rafikiassumani-msft can we add this to the analyzer work for .NET 7?

davidfowl commented 2 years ago

@JamesNK @captainsafia Can we get this one added to the list?

ghost commented 2 years ago

Thanks for contacting us.

We're moving this issue to the .NET 8 Planning milestone for future evaluation / consideration. We would like to keep this around to collect more feedback, which can help us with prioritizing this work. 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.

ghost commented 1 year ago

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.

mkArtakMSFT commented 11 months ago

Guys, bringing this back from Backlog for you to consider this for .NET 9, as we just saw another customer hitting this here: https://github.com/dotnet/aspnetcore/issues/51702

halter73 commented 11 months ago

When we were discussing this in our Web UI planning meeting, we debated whether it makes sense to have a blanket analyzer for all async void usage in Web SDK projects.

I'm sure a lot of usage is okay, but I'd argue most of it is at least a small bug in the context of an ASP.NET Core app. If the async handler is accessing any services, it's completion should be awaited by host shutdown somehow.

If we focus too much on specific problem areas like MVC actions and page handlers, we might miss other problematic issues deep in helper logic. And if you're convinced that your usage is okay because it doesn't rely on the HttpContext and you don't care about ODE's during shutdown (or you're really not relying on any services), you're free to suppress the analyzer.

I personally think we should make our job easier implementing and maintaining the analyzer by flagging any async void methods in a WebSDK project.

dotnet / aspnetcore

Add analyzer to detect async void usage #28217