Test fail when posting to SynchronizationContext.Current

[glconti]

Issue

Our code is relying on a third-party library which posts some action to be executed on the SynchronizationContext.Current, this breaks some tests that require Apartment(ApartmentState.STA) and return async Task.
We managed to strip down the problem to the following tests (one is failing while the other is not).

Tested with NUnit console 3.10.0 and NUnit framework 3.11.0

Failing test

[Test]
[Apartment(ApartmentState.STA)]
public async Task FailingTest()
{
    SynchronizationContext.Current.Post(state => { }, null);

    await Task.CompletedTask;
}

❗️ Exception

System.InvalidOperationException : Shutting down SingleThreadedTestSynchronizationContext with work still in the queue.
   in NUnit.Framework.Internal.SingleThreadedTestSynchronizationContext.ShutDown() in C:\src\nunit\nunit\src\NUnitFramework\framework\Internal\SingleThreadedSynchronizationContext.cs:riga 90
   in NUnit.Framework.Internal.AsyncToSyncAdapter.<>c__DisplayClass10_1.<InitializeExecutionEnvironment>b__0() in C:\src\nunit\nunit\src\NUnitFramework\framework\Internal\AsyncToSyncAdapter.cs:riga 128
   in NUnit.Framework.Internal.AsyncToSyncAdapter.Await(Func`1 invoke) in C:\src\nunit\nunit\src\NUnitFramework\framework\Internal\AsyncToSyncAdapter.cs:riga 88
   in NUnit.Framework.Internal.Commands.TestMethodCommand.RunTestMethod(TestExecutionContext context) in C:\src\nunit\nunit\src\NUnitFramework\framework\Internal\Commands\TestMethodCommand.cs:riga 84

Passing test

[Test]
[Apartment(ApartmentState.STA)]
public async Task SuccessfulTest()
{
    SynchronizationContext.Current.Post(state => { }, null);

    await Task.Yield();
}

Comments

We understood that Task.Yield() will force a context switch allowing for the synchronization context queue to be processed, while Task.Result won't. After having a look at the code and issues we saw that this is linked to #2818 and #2774

[jnm2]

Thanks for reporting. If a test directly or indirectly causes something to be posted to the synchronization context (or thread pool, etc), the test code should wait for the outcome of that action so that it doesn't complete and potentially fail in the context of some other random test.

await Task.Yield(); is an indirect way of waiting for all previous work posted to the context. It's not very nice, but then again, the third-party library is being test-unfriendly by firing and forgetting this work (using SychronizationContext.Post) without providing your code with a way to directly wait for the operation.

Should we fail your test with a more helpful message if you forget to wait for all posted work explicitly within your test, or should we silently continue all posted work and all future work posted by that work until the work dies out for a certain period of time?

For example, what should happen here? A test that runs forever, or some failure like the one you're already getting but more explanatory?

[Test, Apartment(ApartmentState.STA)]
public async Task ProblematicTest()
{
    SynchronizationContext.Current.Post(state => DelayedWork(), null);
}

private void DelayedWork()
{
    SynchronizationContext.Current.Post(state => DelayedWork(), null);
}

➡ I like the idea of explicitly waiting within your test via await Task.Yield(); or some other API we could provide. One reason is that it brings awareness that the code being tested is being difficult, scheduling work out of band without providing any direct way to wait for it. Another reason is that you may want to wait for this work within the test anyway in order to assert something afterwards.

(The way I'm tackling this in my product code is AmbientTasks.Post(...) or AmbientTasks.Add(FooAsync(...)) and then await AmbientTasks.WaitAllAsync(); in my test code. https://gist.github.com/jnm2/ab5624b10efd1ae6fbd6aa8f081a0ec9#file-ambienttasks-cs (soon to be put on nuget.org) and https://gist.github.com/jnm2/0dfcf7dabfb8db76865c124d3a59b20f. Of course, this doesn't help if it's third-party code that's firing and forgetting stuff.)

[jnm2]

(@nunit/framework-team, @glconti, and anyone watching this thread, I'm interested in your input on my questions above.)

[martinskuta]

For example, what should happen here? A test that runs forever, or some failure like the one you're already getting but more explanatory?

It is hard to say what would be the correct way to do it, because of variety of scenarios, that probably only NUnit guys can judge properly. Imo for our case the best solution would be to wait forever, ideally with some timeout exception, because in our case we are using 3rd party messaging framework (MVVM Light) that is using weak references to process message requests and once a request is processed it does a cleanup to remove dead references. And the way it does it is that if the main thread is busy (UI thread - eventhough it is in a test environment), it schedules the work for later on the synchronization context, so the intention is to fire and forget here, we don't really care when it will be done and also don't assert anything related to that cleanup. We would probably care if we were using some static shared messenger, where it would make sense to finish the cleanup to not affect other tests, but this is not our case.

Currently we are using Task.Yield() after test assertion to pass the tests, which is in our view wrong, because you need to know that the tested code is using such messaging framework (which is implementation detail) and that it can fail because of that. For example if we refactor some parts of the code, which are not using messaging, to use that messaging, it can make other tests fail (this is what happend and why we filed this issue). Or on the opposite side, if we replace the messaging with some other framework which is not using synchronization context for cleanup or doesn't need that cleanup, then we would end up with "ghost" Task.Yield calls in tests.

[glconti]

Hey @jnm2 thanks for the insights, (I didn't answer before as I was on vacation), I share @martinskuta's (my colleague) point of view, we'd avoid putting everywhere special code to handle an implementation detail that may change in the future.

[jnm2]

Your reasoning sounds good to me. I'll tackle the issue of recursively scheduled work (see the code block in #3209 (comment)) by implementing a ten second timeout starting from the moment the async method returns.

The timeout will never abort the work currently executing. Rather, it will be enforced in two ways:

• If the current executing work finishes and there is more work in the queue and the timeout has elapsed, an error explaining the situation will be recorded in the test result and the remaining work in the queue will be discarded.

• If SynchronizationContext.Post is called by any thread and the timeout has already elapsed, Post will throw an exception rather than queuing the work and it will record an error in the test result associated with that thread.

Would this satisfy the situations everyone is aware of? I appreciate the feedback.

[jnm2]

I'm leaning towards both setting the thread's test as errored and throwing an exception in both cases. We can always back off later, but otherwise we won't be brought the context to think it through.

[glconti]

Would this satisfy the situations everyone is aware of? I appreciate the feedback.

I think the proposed solution will work, it's perfect for our issue and it's still safe enough to prevent the recursive posting.

[jnm2]

Fixed in 3.12 which is just around the corner!