Deadlocks inside ASP.NET MVC applications

[s-KaiNet]

Category

• Bug

Describe the bug

ASP.NET MVC (.NET Framework-based) application deadlocks when using the library inside synchronous methods.

Steps to reproduce

1. Create a new "old" .NET framework-based ASP.NET MVC web app (ASP.NET Web Application (.NET Framework) template in VS). For the target .NET framework use any 4.x version. Select the MVC option when selecting the type.
2. Add PnP Core SDK NuGet packages and SharePoint CSOM NuGet packages.
3. Change code in, say, Contacts action method inside HomeController:

public ActionResult Contacts()
{
	var services = new ServiceCollection();

	services.AddPnPCore(options =>
	{
        // PnP Office 365 Management Shell = "31359c7f-bd7e-475c-86db-fdb8c937548e";
		options.DefaultAuthenticationProvider = new CredentialManagerAuthenticationProvider("31359c7f-bd7e-475c-86db-fdb8c937548e", "<tenant id>", "cred-name");

		options.PnPContext.GraphFirst = false;
	});

	var provider = services.BuildServiceProvider();
	var scope = provider.CreateScope();

	var pnpContextFactory = scope.ServiceProvider.GetRequiredService<IPnPContextFactory>();

	using (var pnpContext = pnpContextFactory.Create(new Uri(SiteUrl)))
	{
		var list = pnpContext.Web.Lists.GetByServerRelativeUrl("sites/dev/SitePages");
		ViewBag.Message = list.Title;
	}

	return View();
}

4. Run the web app and go to the Contacts page - the web app will deadlock at line GetByServerRelativeUrl.

The given code is used just to illustrate the problem - PnP Core SDK methods are used in a "sync" manner and it leads to the deadlock. Instead GetByServerRelativeUrl you can use Load methods or others. This can be fixed by making controllers methods async, but in our case, we use it not in controllers, but deep inside business logic which cannot be made async.

I debugged I found that deadlock happens exactly at this line. Interesting enough, in my code sample, at line pnpContextFactory.Create the code also sends the request using base.SendAsync, but it doesn't deadlock, but only at GetByServerRelativeUrl afterward.

Expected behavior

I'm not sure what is the expected behavior, because to my understanding ConfigureAwait(false) inside the library helps with the deadlocks inside ASP.NET MVC applications. But for some reasons base.SendAsync deadlocks when used from GetByServerRelativeUrl or Load methods. As I see it, it shouldn't deadlock, but maybe the problem is completely different.

Environment details (development & target environment)

• SDK version: 1.6.0
• OS: Windows 10
• SDK used in: ASP.NET MVC
• Framework: .NET Framework 4.7.2
• Browser(s): any
• Tooling: Visual Studio 2019/22

[s-KaiNet]

Adding sample project for easier reproduction scenario -
WebApplication1.zip

[jansenbe]

@s-KaiNet : could it be that these locks stem from the wrong configureawait() usage that you've fixed via #824 ?

[s-KaiNet]

@s-KaiNet : could it be that these locks stem from the wrong configureawait() usage that you've fixed via #824 ?

Unfortunately no, I thought so, but the code flow doesn't go through those configureawait(true) (to be sure I also debugged with fixed configureawaits and it didn't help). The biggest mystery for me is that the deadlock happens inside await base.SendAsync... which is a call to the native .net framework functions..

[jansenbe]

hmm, thanks for the details @s-KaiNet . I'm back at work as of today, will have some time the coming days to dig into this. Wondering if this would also happen when using .NET 6?

[s-KaiNet]

It works fine in .NET 6 because there no SynchronizationContext (SynchronizationContext.Current equals to null). In "old" ASP.NET MVC projects there is a SynchronizationContext which is AspNetSynchronizationContext. The deadlocks happen because after the async call the .net framework tries to go back to the original thread with AspNetSynchronizationContext, but the original thread is locked waiting for the async method to be completed. As a result, we have a deadlock.

[jansenbe]

@s-KaiNet : running the sync code in a dedicated thread would prevent the deadlock. Would that be work around for you?

public ActionResult Contact()
        {
            Task.Run(() =>
            {
                var services = new ServiceCollection();

                services.AddPnPCore(options =>
                {
                    // PnP Office 365 Management Shell = "31359c7f-bd7e-475c-86db-fdb8c937548e";
                    options.DefaultAuthenticationProvider = new CredentialManagerAuthenticationProvider("31359c7f-bd7e-475c-86db-fdb8c937548e", TenantId, CredName);

                    options.PnPContext.GraphFirst = false;
                });

                var provider = services.BuildServiceProvider();
                var scope = provider.CreateScope();

                var pnpContextFactory = scope.ServiceProvider.GetRequiredService<IPnPContextFactory>();

                using (var pnpContext = pnpContextFactory.Create(new Uri(SiteUrl)))
                {
                    var list = pnpContext.Web.Lists.GetByServerRelativeUrl(ListUrl);
                    ViewBag.Message = list.Title;
                }
            });

            return View();
        }

[s-KaiNet]

Yeah, that would work. I just try to understand why we have so weird behavior in the "normal" sync case. SynchronizationContextRemover also works in this case, but maybe it's not the best solution.

[jansenbe]

Not sure if we need to change PnP Core SDK for this purpose @s-KaiNet , but I'm happy to listen to your arguments if you feel changes are needed. We encourage folks to use the async PnP Core SDK methods and the main usage of the library is with .NET Core and .NET 5/6 based applications. I've checked the Azure .NET SDKs as they offer sync and async as well and they use the same GetAwaiter().GetResult() approach. Their docs: https://azure.github.io/azure-sdk/dotnet_introduction.html#sync-and-async explain that and also point to an article articulating the potential async deadlocks this can lead to.

We could consider wrapping all our method().GetAwaiter().GetResult() calls via Task.Run(), (see https://stackoverflow.com/a/59019473) but that could still result in threadpool starvation issues under load. Think it's better that developers that need to use sync implement Task.Run() themselves (like shown above and below).

What if we clearly spell this out in documentation: if you're stuck with deadlocks when using sync method calls then consider wrapping them in Task.Run()?

using (var pnpContext = Task.Run(() => pnpContextFactory.CreateAsync(new Uri(SiteUrl))).GetAwaiter().GetResult())
{
    var list = Task.Run(() => pnpContext.Web.Lists.GetByServerRelativeUrlAsync(ListUrl)).GetAwaiter().GetResult();
    ViewBag.Message = list.Title;
}

[s-KaiNet]

I also don't think we need change anything in the library. My scenario is more about some legacy stuff like old asp.net MVC. I though that maybe I did something wrong and the fix is fairly simple. But anyway as I have some workarounds I'm ok with that. I think we can close the issue.

[jansenbe]

@s-KaiNet : Did add a small note to our docs (https://pnp.github.io/pnpcore/using-the-sdk/basics-async.html) for future reference. Closing the issue now. Thanks for pushing this issue as it helps to improve the quality of the SDK.