Html.Partial causes deadlocks and should be marked obsolete

[hach-que]

So I've spent the last couple of days trying to figure out the source of deadlocks in our application, and it turns out that Html.Partial can cause deadlocks in ASP.NET Core 2.0, because it's implementation is:

var result = htmlHelper.RenderPartialAsync(partialViewName, htmlHelper.ViewData.Model, viewData: null);
result.GetAwaiter().GetResult();

which is a classic misuse of async/await APIs due to the potential for deadlocks when you do synchronous blocking on a task.

Literally in our Razor view, changing:

@Html.Partial("../Shared/Navigation", new JamHost.ViewModels.NavigationViewModel
{
  Entries = JamHost.Services.NavigationEntries.GetNavigationEntries(),
  User = CurrentUser.IsAuthenticated ? await CurrentUser.GetUser() : null
})

to

@await Html.PartialAsync("../Shared/Navigation", new JamHost.ViewModels.NavigationViewModel
{
  Entries = JamHost.Services.NavigationEntries.GetNavigationEntries(),
  User = CurrentUser.IsAuthenticated ? await CurrentUser.GetUser() : null
})

is the difference between a deadlock and the page rendering okay, which is crazy because as a consumer, it means I can't trust any of the MVC API surface when it comes to "will using this function cause deadlocks?". There's no declaration or warning that using Html.Partial in Razor views can cause deadlocks, and the only reason I even thought of changing Html.Partial to Html.PartialAsync as a solution is because someone else also ran into this problem. Indeed at the moment we're fighting random scenarios where our application deadlocks and never continues serving any more requests, and because of this issue I'm likely to believe it is in fact just that half the MVC API surface we're using can cause deadlocks because of an implementation detail.

Anyway, any methods which can cause deadlocks like this should either be fixed, or be marked Obsolete telling the consumer to use the async version (as far as I'm aware all Razor views are async now anyway so there's no scenarios where the non-async versions should be used).

[Eilon]

@hach-que sorry you ran into this, we'll investigate what's going on here.

@rynowak can you investigate when you return from The Island?

[rynowak]

This seems like a good candidate for an analyzer. I can't think of a reason why we'd tell someone you should use the non-async version. I also can't see us deleting it from the universe. The .NET team has been introducing analyzers for these cases rather than using [Obsolete]. https://blogs.msdn.microsoft.com/dotnet/2017/10/31/introducing-api-analyzer/ - See the first comment and reply from @terrajobst for rationale.

So I've spent the last couple of days trying to figure out the source of deadlocks in our application, and it turns out that Html.Partial can cause deadlocks in ASP.NET Core 2.0

@hach-que - are you calling any ASP.NET Core code from inside of a sync-context? what code introduced the sync-context?

We don't generally account for sync contexts anywhere in ASP.NET Core - which is a change from older ASP.NET. We might need some guidance or warnings about this. I wonder if it's possible for us to provide some diagnostics if you call one of these problematic stacks in a sync context.

Doing a scan of our code, all of the problematic cases are in view rendering. https://github.com/aspnet/Mvc/search?utf8=%E2%9C%93&q=GetAwaiter&type=

[hach-que]

What's happening here is

• Razor executes the view using an async call.
• The view includes a partial synchronously, which internally calls Wait on a task (because Partial on it's own is not async).
• There are no more task slots available, so you get a deadlock (the view can't complete because it's synchronously Waiting on the partial internally, but the partial can't complete because the task can't be scheduled).

This scenario occurs when there's already a bunch of other tasks consuming the available task slots - especially if you're doing non-async Partial in like the base layout, it's pretty easy to run into deadlocks with this situation.

It's not safe to use Wait on a task if another async method is higher in the chain of method calls, which for ASP.NET Core is pretty much everywhere these days.

[benaadams]

It's not safe to use Wait on a task if another async method is higher in the chain of method calls, which for ASP.NET Core is pretty much everywhere these days.

I've made a BlockingDetector package to output warnings to the logs when you block:
https://github.com/benaadams/Ben.BlockingDetector

Only detects when blocking actually happens; so doesn't pick up calls that may block but don't or warn about coding practices that lead to blocking (or blocking that happens at OS rather than .NET level e.g. File.Read) - but it may help pick up things.

[mkArtakMSFT]

@rynowak, what are the next steps here? Should we file a separate issue to introduce a breaking change in the next major release (3.0+) and get rid of these synchronous wrapper APIs?

[rynowak]

I think we need to have a broader discussion about what to do here long term. On second look this happens for templates (EditorFor/DisplayFor).

Mvc/src/Microsoft.AspNetCore.Mvc.ViewFeatures/ViewFeatures/TemplateRenderer.cs

Line 116 in 5fffd46

public IHtmlContent Render()

I think we might need to consider some pretty bold moves. Trying to totally eliminate all GetResult() from the view engine will really really really churn people's apps. This is not limited to just Html.Partial

[rynowak]

Kicking this back to SUPER TRIAGE

[mkArtakMSFT]

So here what we're going forward with:

• For the current release, we're planning to release an Analyzer, to warn users from using Html.Partial: Create a new Analyzer to warn users from using Html.Partial #7417
• The long term fix will be handled as part of https://github.com/aspnet/Mvc/issues/7416

[mkArtakMSFT]

Closing this one in favor of two referenced items.