aspnetcore icon indicating copy to clipboard operation
aspnetcore copied to clipboard

Add analyzer to detect async void usage

Open davidfowl opened this issue 5 years ago • 11 comments

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

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

davidfowl avatar Nov 28 '20 03:11 davidfowl

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.

ghost avatar Nov 30 '20 09:11 ghost

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

Youssef1313 avatar Feb 11 '21 13:02 Youssef1313

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.

JunTaoLuo avatar May 05 '21 17:05 JunTaoLuo

@Youssef1313 are you still interested?

davidfowl avatar Jun 03 '21 07:06 davidfowl

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

Youssef1313 avatar Jun 06 '21 19:06 Youssef1313

Hub methods could also be included here.

BrennanConroy avatar Jun 06 '21 20:06 BrennanConroy

Added

davidfowl avatar Jun 06 '21 20:06 davidfowl

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.

ghost avatar Aug 10 '21 17:08 ghost

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

davidfowl avatar Nov 02 '21 06:11 davidfowl

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

davidfowl avatar Oct 01 '22 06:10 davidfowl

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 avatar Oct 11 '22 19:10 ghost

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.

ghost avatar May 09 '23 23:05 ghost

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

mkArtakMSFT avatar Dec 18 '23 18:12 mkArtakMSFT

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.

halter73 avatar Dec 18 '23 18:12 halter73