Remove blocking APIs in asp.net core integration tests code

[gdoron]

Is there an existing issue for this?

• [X] I have searched the existing issues

Describe the bug

Even on our very powerful Macbook pros, when we are running one of our integration test projects that has hundreds of tests, many of which use WebApplicationFactory we often get into deadlocks.

We set xunit to use unlimited amount of threads (-1), we have overridden a bunch of xunit stuff to implement a semaphore to limit the number of concurrent tests. And yet, often we get into deadlocks after the ~600 tests completed mark. After verifying we have no sync-over-async, we found this beauty in asp.net core code itself...

https://github.com/dotnet/aspnetcore/blob/main/src/Hosting/TestHost/src/TestServer.cs#L101 As well as this: https://github.com/dotnet/runtime/blob/2be87c9c976c0cd21b66ea12ae406a559aacaac0/src/libraries/Microsoft.Extensions.Hosting.Abstractions/src/HostingAbstractionsHostExtensions.cs#L19

later on when we managed to reproduce the deadlock (after a very high number of attempts).

Bear in mind, xUnit which is by far the number 1 testing framework for .NET Core has SynchronizationContexts, even if this issue would be solved, there is still the MaxConcurrencySyncContext which I can only assume would still be used by default.

Can we please remove the sync over async anti pattern from our beloved framework?

Best, Doron

[Tratcher]

That TestServer constructor is no longer necessary, it was replaced by the following and only remains for back-compat, we could consider obsoleting it. https://github.com/dotnet/aspnetcore/blob/48a81a70597fa6726d0c506692a316e98038a442/src/Hosting/TestHost/src/WebHostBuilderExtensions.cs#L24

That Start extension method is also not necessary, it's only there for convenience. In a normal app it would only be called once and would be harmless. I agree it is a bit of a trap though.

[gdoron]

@Tratcher I'm not sure I understand. We are not talking about normal app, we are talking about tests project, that runs multiple tests in parallel, each with its own TestServer. So it's not harmless at all. I probably did not understand what you meant.

[Tratcher]

I meant it's harmless when the app is run in a development or production environment where there's only one instance per process. I see how it's problematic in a parallel test environment with many instances.

[adityamandaleeka]

Triage: we could do a bunch of ConfigureAwait(false) on these things.

[gdoron]

@adityamandaleeka no way of making it truly async? Is the best possible a blocking ConfigureAwait(false)?

[Tratcher]

@gdoron async versions of these APIs already exist. The only thing we can change here is discouraging people from using the old sync ones.

[gdoron]

@Tratcher how can one use the async versions when using \ deriving from WebApplicationFactory? https://docs.microsoft.com/en-us/aspnet/core/test/integration-tests?view=aspnetcore-6.0#customize-webapplicationfactory

[Tratcher]

Fair point, I hadn't reviewed WebApplicationFactory.

This usage looks async safe: https://github.com/dotnet/aspnetcore/blob/b883b451b02876b9adaf3c4ce7ae0ac441f471bb/src/Mvc/Mvc.Testing/src/WebApplicationFactory.cs#L210

This one looks unsafe, CreateHost should be async: https://github.com/dotnet/aspnetcore/blob/b883b451b02876b9adaf3c4ce7ae0ac441f471bb/src/Mvc/Mvc.Testing/src/WebApplicationFactory.cs#L437

[gdoron]

@Tratcher any chance having this sorted for 8.0 or even a minor version of 7?

[Tratcher]

Yes, we plan to look at it in 8. If you want to try fixing it that would be helpful.

[gdoron]

@Tratcher just making sure this was not forgotten. These deadlocks are a huge PITA for us for over a year now. (Unfortunately I'm not knowledgeable enough to fix it myself 😞)

[spaasis]

Possible workaround mentioned here https://www.strathweb.com/2021/05/the-curious-case-of-asp-net-core-integration-test-deadlock/

Subclass WebApplicationFactory:

private class AsyncFriendlyWebApplicationFactory<T> : WebApplicationFactory<T> where T : class
{
    protected override IHost CreateHost(IHostBuilder builder)
    {
        var host = builder.Build();
        Task.Run(() => host.StartAsync()).GetAwaiter().GetResult();
        return host;
    }
}

[ardalis]

This would still be a huge improvement to integration tests, especially those that use async methods to seed data. Ideally we would add a CreateHostAsync method that returns Task<IHost> and would thus allow us to await any method we needed to within that method.

[amcasey]

@ardalis I doubt we'll be able to get to it in 9.0, but I've flagged it for review in 10.0.