Opting out of AsyncTestSyncContext

[gdoron]

Hi All,

Everyone today's under the impression we are done with deadlocks because of SynchronizationContexts since they were removed in ASP.NET Core.
Problem is, the same code that works perfectly in development, staging and production is causing very weird problems and even deadlocks when run inside xunit tests.
This is because xunit added 2 SynchronizationContexts:

• MaxConcurrencySyncContext for limiting the number of threads
• AsyncTestSyncContext to keep track of async void tests

While the former can easily be opted-out, with MaxParallelThreads=-1, with AsyncTestSyncContext since it's being used very deep inside the framework in https://github.com/xunit/xunit/blob/v2/src/xunit.execution/Sdk/Frameworks/Runners/TestInvoker.cs#L238 it seems like you simply cannot opt-out unless you override a half of the framework.

I noticed this is fixed with v3, where we only get AsyncTestSyncContext if the test method is in fact async void: https://github.com/xunit/xunit/blob/main/src/xunit.v3.core/Sdk/v3/Runners/TestInvoker.cs#L143

Personally, I would throw an exception for async void, aligning xunit with .NET's other test frameworks that do not support async void (I honestly see no advantage of having async void, probably a typo and can be fixed in a second or with a linter but many many disadvantages).

Can we please add a reasonable way of opting-out of AsyncTestSyncContext in the production V2 branch?
If investing a lot of time in it is not desirable since it's already fixed in V3, we can very easily just look at an environment variable in TestInvoker.

Bear in mind:

• Finding the root cause of these deadlocks is very difficult and time consuming
• ASP.NET Core integration tests framework shockingly is using sync over async in its code base, that making deadlocks super likely in big projects. See this thread: Tests hang from dotnet test microsoft/vstest#2080 (comment)
• Code under test should run and behave as much as possible as code outside the test, putting code inside a SynchronizationContext for tests, is a huge and impactful change.

Best,
Doron Grinzaig

[gdoron]

@bradwilson sorry for the rude ping.
But any chance you can review the issue?

Thanks in advance!

[bradwilson]

and even deadlocks

You can deadlock the max concurrency context, which you already have a way to opt out of.

Looking at the code, I cannot see how you could deadlock the async void context itself, though it does delegate to the context that is wraps (so if you're using a context which could be deadlocked, like one of the STA/UI thread extensions, that's what you're deadlocking, and it has nothing to do with the async void context). If you can reliably deadlock the async void context by itself (meaning, no other contexts, including the max concurrency context), please provide a repro project.

The v3 bypass of the async void context has everything to do with performance and nothing to do with potential deadlocks.

Personally, I would throw an exception for async void

This is not going to happen. Supporting async void is a pit of success feature that's been in the product for many many years. I'm not arbitrarily removing it.

Can we please add a reasonable way of opting-out of AsyncTestSyncContext in the production V2 branch?

You can send a PR if you wish, but I have not promised there will be any more v2 releases, so you may be stuck using a MyGet-based package to get this feature for an indefinite period of time.

[bradwilson]

This will not be fixed for v2.

For v3, we only use the AsyncTestSyncContext when your test method is async void; as such, the opt-out mechanism in v3 is "don't write async void tests".

xunit/src/xunit.v3.core/Sdk/v3/Runners/TestInvoker.cs

Lines 143 to 148 in e138f20

	if (AsyncUtility.IsAsyncVoid(ctxt.TestMethod))
	{
	oldSyncContext = SynchronizationContext.Current;
	asyncSyncContext = new AsyncTestSyncContext(oldSyncContext);
	SetSynchronizationContext(asyncSyncContext);
	}

As such, I consider this issue resolved.

[neuecc]

With the release of TimeProvider in .NET 8, asynchronous operations involving time can now be processed synchronously. However, the insertion of this SynchronizationContext inadvertently leads to the use of ThreadPool, making it difficult to monitor the processes. I am working on a custom implementation of Reactive Extensions, and this behavior is very inconvenient when writing tests for complex async/await code.

It's uncertain when version 3 will be released and become widespread, so I would like it to be introduced in version 2 before moving to version 3.

[bradwilson]

Available for v2 in 2.6.7-pre.11 https://xunit.net/docs/using-ci-builds

[gdoron]

@bradwilson thanks! If I got the commit right, it's not opt-out, it's opt-in, meaning you'll get the sync context if you choose to use async void.
Right?
If so, this is by far the best option, thank you!

[bradwilson]

If I got the commit right, it's not opt-out, it's opt-in, meaning you'll get the sync context if you choose to use async void.

It's really a matter of perspective, and whether you're accustomed to using async void with xUnit.net. Most people don't use it elsewhere except where they have to.

[mconnew]

FYI, this was a breaking change for CoreWCF as we have tests which depended on a non-null sync context to be set (we're validating whether we propagate it through to service call dispatch based on a behavior setting). Using the class name SynchronizationContext in the release notes item for this change would have made this easier to find when doing a contextual search to try work out what changed.

[bradwilson]

@mconnew It's unfortunate that we caused this bug, but depending on our (internal implementation detail) sync context instead of providing your own isn't really our fault.

Using the class name SynchronizationContext in the release notes item for this change would have made this easier to find

Our release notes are pretty shallow, and we did call out the exact class in question (this is the third item in the release notes for 2.7.0):

It's unfortunately impossible for me to have both anticipated that you were taking this dependency, and then how you would search to find the problem. I believe I provided as much information as most people would reasonably expect. 🤷🏼

[bradwilson]

Note that based on feedback from several users across several issues, this feature is being rolled back in 2.8.0, alongside the release of a new parallelism algorithm.

At the same time, we have removed support for async void tests in v3, so the sync context will be removed entirely. New analyzers in 1.13.0 will flag async void tests with a warning in v2 projects, and an error in v3 projects (and any attempt to run them in v3 will cause fast failures for the tests in question).