Implemented the ability to await in STA tests 🎉 #2818

jnm2 · 2018-04-21T05:53:54Z

Whew! This was fun and the collateral was worth doing, but this was a longer time coming than I expected!

The heart and soul of this change is the 8th (last) commit. Its motivating tests are the 6th commit.

The 7th commit took a lot of thought to get right while keeping the complexity down. It refactors AsyncInvocationRegion—which as I've noticed many times since I arrived at this project, had become neither async, an invocation, nor a region 😆—due to the necessity for it to control the invocation itself rather than being handed the return value from the invocation. I did my best to come up with a descriptive name, AsyncToSyncAdapter, since it adapts async execution to synchronous execution.

The preceding commits were things which were all in the critical path, so for simplicity they are in the same PR. They should each be viewed separately.

@rprouse Would it be a good idea for me to do a walkthrough explanation of the 8th commit for posterity, or do #1200 and #2774 explain it well enough?

/cc @sharwell

…synchronization context check (saving thread check for later when STA synchronization context is added, plus it overlaps with the next commit's tests)

rprouse · 2018-04-24T23:24:39Z

@jnm2 I am not neglecting this, I just sold a house AND I'm swamped at work right now. I will review when my head is clearer.

jnm2 · 2018-04-24T23:40:39Z

@rprouse I appreciate that. Don't worry about it! I also have my hands full. I'd rather have your clear-headed review. 😜 And I don't mind having an excuse to do better documentation or write this one up.

(I'm thinking: I've been programming lazily and hoping people will just tell me where my code is hard to understand so that I can improve it. But that may not be fair on you...)

…lResetEventSlim polyfills for net35 performance

jnm2 · 2018-04-28T17:00:16Z

I'm adding a simple ManualResetEventSlim polyfill in this one, but I built a commit on top of it with full-blown lock-free CountdownEvent and spinning/lazy ManualResetEventSlim to see if that improves the throughput and gets rid of all the performance issues on net35 CI.

After this PR is merged I'll rebase it and start a separate one.

…lResetEventSlim polyfills for net35 performance

rprouse

Looks good to me although it is a lot to digest. I've asked a couple of questions, so I am not approving yet. @ChrisMaddock or @mikkelbu are either of you up to a second review? This is a large change, so the more eyes the better.

rprouse · 2018-05-03T00:23:20Z

src/NUnitFramework/framework/Assert.Exceptions.cs

-            }
-            else
-#endif
+            Guard.ArgumentNotAsyncVoid(code, nameof(code));


Shouldn't we be guarding that code shouldn't be async period?

code is TestDelegate which is declared as delegate void, so if it's ever async, it's async void. I added a comment to make that clear.

rprouse · 2018-05-03T00:26:23Z

src/NUnitFramework/framework/Internal/AsyncToSyncAdapter.cs

+            {
+                if (type.GetTypeInfo().IsGenericType
+                    ? type.GetGenericTypeDefinition().FullName == "System.Threading.Tasks.Task`1"
+                    : type.FullName == "System.Threading.Tasks.Task")


What about ValueTask?

I'm thinking of this as the second async PR, and had planned ValueTask along with general awaitable support in the third PR.

rprouse · 2018-05-03T12:57:36Z

.NET 3.5 tests hung on AppVeyor, I've kicked off a new build.

jnm2 · 2018-05-05T01:58:43Z

Looks good to me although it is a lot to digest. I've asked a couple of questions, so I am not approving yet. @ChrisMaddock or @mikkelbu are either of you up to a second review? This is a large change, so the more eyes the better.

I would like a second one. Rob, from your review, do you think it would benefit from me commenting the code?

ChrisMaddock · 2018-05-05T19:36:41Z

I can try and have a look next week - but I know very little about .NET async, so am probably the wrong person for it!

jnm2 · 2018-05-05T20:07:45Z

@ChrisMaddock Thanks for being willing to look! I can think of two-way benefits that can come from reviewing even so. You might catch a typo or think of an NUnit async constraint I didn't cover, and this way you have a general sense of where code is and what it's called. And if you find yourself curious about async, I enjoy explaining things like that. (Whether I'm good at explaining it might be another question. 😄)

jnm2 · 2018-05-05T20:42:05Z

@ChrisMaddock I wrote it up for you anyway! 😄

All you need to know about async for this PR:

When you use await foo, your method returns back to the caller without running the rest of the method.
Instead, your method wraps the rest of itself in an Action so that it can ask foo to schedule that Action to run when foo is completed. foo can schedule this however it wishes.
When foo is a Task, foo avoids invoking the rest-of-the-method Action directly when it completes because foo might be being completed by a different thread. (Think TaskCompletionSource.SetResult.)
Instead, foo uses SynchronizationContext.Current.Post to schedule the Action so that the rest of the method executes in the same context¹ as the part of the method before the await. (This is why after awaiting in Windows Forms, the rest of your method executes on the main UI thread instead of executing on the thread that called TaskCompletionSource.SetResult.)

And all you need to know about SynchronizationContext:

The default synchronization context doesn't synchronize anything. Everything you Post to it it immediately turns around and gives to ThreadPool.QueueUserWorkItem. This is why if you are on an STA thread without a synchronization context and you await, the rest of your method runs on a random non-STA background thread. This is the problem this PR fixes.
¹ ‘Context’ means whatever you want it to, if you are the implementer of a SynchronizationContext. People ask you to run stuff by calling your implementation of Post. You get to decide if you want to run each action now or later, or eleven times, or not at all, or get a particular thread to do it, etc.
To fix the problem, I just implemented a SynchronizationContext which throws everything in a queue. Later, while NUnit's STA worker thread is waiting for the async method to finish, rather than blocking the whole time, the STA worker thread gets busy executing everything in the queue until the test method is finished.

The rest is plumbing: AsyncToSyncAdapter (formerly AsyncInvocationRegion) gets a SingleThreadedTestMessagePumpStrategy and calls WaitForCompletion which is implemented exactly like the Windows Forms and WPF strategies.

And to get the test method to await using our synchronization context instead of the default one, we have to set the synchronization context before invoking the test method.

jnm2 · 2018-05-05T20:44:55Z

@rprouse One of these days I'm going to write it up properly in the PR body. 😆

ChrisMaddock · 2018-05-07T18:51:28Z

Thanks Joseph, that looks great! I'll try and take a look at this next week. 😄

jcansdale · 2018-05-08T00:20:16Z

Hi folks. I thought this might be of interest/relevant:
https://github.com/AArnott/Xunit.StaFact

This uses distinct StaFact, UIFact, WpfFact and WinFormsFact attributes to more fully emulate different application contexts. I wonder if this PR is similar to the UIFact attribute? I haven't had a chance to drill into the details.

The examples given for these attributes are:

[WpfFact]
public async Task WpfFact_OnSTAThread()
{
    Assert.Equal(ApartmentState.STA, Thread.CurrentThread.GetApartmentState());
    await Task.Yield();
    Assert.Equal(ApartmentState.STA, Thread.CurrentThread.GetApartmentState()); // still there
}

[UIFact]
public async Task UIFact_OnSTAThread()
{
    int initialThread = Environment.CurrentManagedThreadId;
    await Task.Yield();
    Assert.Equal(initialThread, Environment.CurrentManagedThreadId);
}

jnm2 · 2018-05-08T00:35:08Z

I see this PR solves the same problem UIFact does, while #2776 solves what WinFormsFact and WpfFact do. I took a more hands-off approach. E.g. rather than installing a WindowsFormsSynchronizationContext, it just reacts compatibly to the one you have.

ChrisMaddock

Thanks for the write-up! I don't think I'm familiar enough with this stuff to notice if there were any glaring errors - but I was able to follow it through. 🙂

ChrisMaddock · 2018-05-11T20:49:22Z

src/NUnitFramework/tests/TestUtilities/DummyConstraint.cs

@@ -0,0 +1,15 @@
+using NUnit.Framework.Constraints;


Just a header missing here

👍 Fixed.

ChrisMaddock · 2018-05-11T21:05:00Z

src/NUnitFramework/framework/Internal/AsyncToSyncAdapter.cs

+        private const string TaskResultProperty = "Result";
+        private const string VoidTaskResultType = "VoidTaskResult";
+        private const string SystemAggregateException = "System.AggregateException";
+        private const string InnerExceptionsProperty = "InnerExceptions";


Are these strings mostly just used the once? Is there a reason not to inline them?

Spot on. I'm going to leave it since this code was moved without changing it, and in the next PR when I add support for general C# awaitables (and maybe F# too), I'll be pulling this stuff out anyway.

ChrisMaddock · 2018-05-12T00:15:06Z

src/NUnitFramework/tests/AsyncExecutionApiAdapter.cs

+            new ThrowsConstraintAdapter(),
+            new ThrowsExceptionConstraintAdapter(),
+            new NormalConstraintAdapter()
+        };


I like these! 😄

Yeah, isn't this better than what I tried to do the last time? 😆
Oops, I just realized I forgot to cover setups and teardowns!

…synchronization context check (saving thread check for later when STA synchronization context is added, plus it overlaps with the next commit's tests)

… invocation

jnm2 · 2018-05-12T02:51:19Z

@rprouse, @ChrisMaddock I rebased to fix the missing header Chris found, and then I added the last four commits containing some new simple tests and refactors. As soon as any team member reapproves, I'm happy for this to be merged.

jcansdale · 2018-05-13T20:50:59Z

I was getting some strange results when using NUnit to test some related functionality in TerstDriven.Net (see jcansdale/TestDriven.Net-Issues#110 for the gory details).

It turns out the following test fails in NUnit 3.10.1:

[Test]
[Apartment(ApartmentState.STA)]
public async Task ApartmentStateAfterYield()
{
    await Task.Yield();
    Assert.That(Thread.CurrentThread.GetApartmentState(), Is.EqualTo(ApartmentState.STA));
}

This test passes when using the nunit.framework from this PR.

This PR would get a ✔️ from me. Thanks for your work on this @jnm2. I still find the task architecture a little mind bending and your write-ups have been appreciated. 👍

BTW, I think this would make a great blog post and something for the NUnit twitter. What do you reckon @jnm2? 😄

jnm2 · 2018-05-13T22:54:44Z

@jcansdale

This PR would get a ✔️ from me. Thanks for your work on this @jnm2. I still find the task architecture a little mind bending and your write-ups have been appreciated. 👍

Thanks! I see you're a member here, which is awesome... I don't know but I don't think there would be a problem with you approving the PR if you wanted?
My explanations are only oversimplifications of what I've absorbed from smarter folks years ago. I want to democratize async. 😁

BTW, I think this would make a great blog post and something for the NUnit twitter. What do you reckon @jnm2? 😄

Haha, I had just offered to help with this. There are at least three more fundamental changes beyond these two which I want to get in for 3.11:

support for any C# awaitables (last item in SynchronizationContexts and awaiting #2774)
- maybe support for F#'s async
save and restore the synchronization context along with the test execution environment so that one test can never affect another (Save and restore the SynchronizationContext before and after each test case #2821)
Very hopeful: introduce an AsyncAssert class (Replacing ThrowsAsync with a composable async alternative #2843)

And a sweep of a dozen potentially-related existing issues.

jnm2 · 2018-05-13T22:58:27Z

However, @rprouse, don't wait for my my slow pace with these if you want to release 3.11 sooner. Master has been continuously deliverable as far as I know.

jcansdale

Tested this build with an async test I was having problems with. This PR makes STA tests behave is a much less surprising way.

Haven't had a chance to review the code itself.

jcansdale · 2018-05-14T08:54:03Z

Thanks! I see you're a member here, which is awesome... I don't know but I don't think there would be a problem with you approving the PR if you wanted?

I've approved from an API consumer PoV. I'd love to review the code itself, but work, family etc. 😕

jnm2 · 2018-05-14T11:09:54Z

@jcansdale Understandable, no worries. Thanks for your feedback!

rprouse · 2018-05-14T15:41:59Z

LGTM, let's get this merged 😄

jnm2 · 2018-05-14T23:54:18Z

If you'd like to take advantage of this early, look for version 3.11.0-dev-05580 or greater when you merge this with your nuget.config:

<?xml version="1.0" encoding="utf-8"?>
<configuration>
  <packageSources>
    <add key="NUnit MyGet" value="https://www.myget.org/F/nunit/api/v3/index.json" />
  </packageSources>
</configuration>

Belorus · 2018-05-27T09:14:02Z

Hi @jnm2

I tried package 05600 with your changes.
#2851 - here i get
System.InvalidOperationException : Shutting down SingleThreadedTestSynchronizationContext with work still in the queue.
This behavior makes perfect sense in very strict env where you wish to avoid async void methods anywhere in your code.

It would be really amazing to make a new configurable behavior that executes all delegates in SynchronizaitonContext that are left. This will make testing of async void methods possible.

WDYT ?

Belorus · 2018-05-27T09:19:15Z

Also i've noticed that

[assembly: Parallelizable(ParallelScope.Fixtures)]
[assembly: Apartment(ApartmentState.STA)]

is 3-5 times slower than just

[assembly: Parallelizable(ParallelScope.Fixtures)]

CharliePoole · 2018-05-27T09:30:57Z

@Belorus Since there's only one STA, your first example is single-threaded for all practical purposes. If you had some non-STA fixtures, they would run on parallel. Are you sure all your tests need the STA?

Belorus · 2018-05-27T09:41:38Z

n@CharliePoole Well, what my example needs is a SynchronizationContext that will run all delegates in its queue before letting test finish. This means that all async void methods have finished their work.

This sounds for me as a good behavior for most cases, if you expect people testing async void methods.

From my POV its absolutely legal to create 8 threads each with its own SynchronizationContext. I don't see reason to execute such tests sequentially.

CharliePoole · 2018-05-27T10:07:25Z

Because STA is an old COM concept it may not be what you want. @jnm2 did all the recent work on synchronization contexts and will probably have to help us out here.

jnm2 · 2018-05-27T11:56:29Z

@Belorus Thanks for the questions! I replied about async void over where you initially asked: #2851 (comment).

Also i've noticed that

[assembly: Parallelizable(ParallelScope.Fixtures)]
[assembly: Apartment(ApartmentState.STA)]

is 3-5 times slower than just

[assembly: Parallelizable(ParallelScope.Fixtures)]

I would expect NUnit to be creating eight STA threads for you. So long as nothing COM-sensitive crosses from one thread to another, that's the ideal situation.
(Easy to avoid- don't put anything COM-sensitive into state that is shared between the threads, such as an instance field of a fixture with ParallelScope.Children.)

jnm2 · 2018-05-27T12:21:15Z

Also, Charlie is right about STA being an old COM thing. You should only use it if you need to use COM, such as certain Windows UI. If what you really want is for your async test to stay on the same thread, I think the thing that aligns 1:1 with your goal is actually a SingleThreadedSynchronizationContext like the one I posted at #2851 (comment) or my original one at #1200 (comment).

jnm2 requested a review from rprouse April 21, 2018 05:53

jnm2 mentioned this pull request Apr 21, 2018

[WIP] Using C# awaitable pattern for test methods and delegates #2181

Closed

jnm2 added the awaiting:review label Apr 21, 2018

jnm2 added 3 commits April 23, 2018 18:13

Add compatibility extension method Delegate.GetMethodInfo

2532f83

Added compatibility shim for ManualResetEventSlim

dfcd27e

More thorough async API tests, replaced thread check with equivalent …

326bdc5

…synchronization context check (saving thread check for later when STA synchronization context is added, plus it overlaps with the next commit's tests)

jnm2 force-pushed the jnm2/sta_synchronizationcontext branch from 0c002f0 to 2e5cb68 Compare April 23, 2018 22:15

jnm2 added a commit that referenced this pull request Apr 28, 2018

[todo: after #2818 is merged] Lock-free CountdownEvent and lazy Manua…

213a471

…lResetEventSlim polyfills for net35 performance

jnm2 added a commit that referenced this pull request Apr 28, 2018

[todo: after #2818 is merged] Lock-free CountdownEvent and lazy Manua…

46381f4

…lResetEventSlim polyfills for net35 performance

jnm2 mentioned this pull request Apr 29, 2018

Save and restore the SynchronizationContext before and after each test case #2821

Closed

rprouse reviewed May 3, 2018

View reviewed changes

jnm2 force-pushed the jnm2/sta_synchronizationcontext branch from 2e5cb68 to 4be14d6 Compare May 3, 2018 03:03

rprouse previously approved these changes May 3, 2018

View reviewed changes

jcansdale mentioned this pull request May 7, 2018

[STAThread] async Task hangs when executed with Test With > In-Proc (VS SDK) jcansdale/TestDriven.Net-Issues#110

Closed

Belorus mentioned this pull request May 11, 2018

Tests are green with exceptions thrown in async void inner method (test itself is async Task) #2851

Closed

ChrisMaddock reviewed May 11, 2018

View reviewed changes

ChrisMaddock reviewed May 12, 2018

View reviewed changes

More thorough async API tests, replaced thread check with equivalent …

1f7665b

…synchronization context check (saving thread check for later when STA synchronization context is added, plus it overlaps with the next commit's tests)

jnm2 added 7 commits May 11, 2018 22:47

Failing tests for awaits from STA continuing on same thread

9d88355

Necessary to refactor AsyncInvocationRegion to give it control of the…

b83f556

… invocation

Implemented the ability to await in STA tests

75afc1e

Added tests on setup and teardown async entry points

a99f4cf

Organized AsyncExecutionApiAdapter implementations into files

e36914c

Make sure async API tests are testing what we think they’re testing

92e2940

Use TaskEx uniformly for easier reading (thanks @sharwell)

728b6cb

jnm2 dismissed rprouse’s stale review via 728b6cb May 12, 2018 02:47

jnm2 force-pushed the jnm2/sta_synchronizationcontext branch from 4be14d6 to 728b6cb Compare May 12, 2018 02:47

Async API adapter fixtures need to be in TestData assembly

3b065db

jcansdale approved these changes May 14, 2018

View reviewed changes

rprouse approved these changes May 14, 2018

View reviewed changes

rprouse merged commit 5a3d7f8 into master May 14, 2018

rprouse deleted the jnm2/sta_synchronizationcontext branch May 14, 2018 15:42

glconti mentioned this pull request Apr 17, 2019

Test fail when posting to SynchronizationContext.Current #3209

Closed

Implemented the ability to await in STA tests 🎉 #2818

Implemented the ability to await in STA tests 🎉 #2818

Conversation

jnm2 commented Apr 21, 2018

rprouse commented Apr 24, 2018

jnm2 commented Apr 24, 2018

jnm2 commented Apr 28, 2018 • edited Loading

rprouse left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

rprouse commented May 3, 2018

jnm2 commented May 5, 2018

ChrisMaddock commented May 5, 2018

jnm2 commented May 5, 2018

jnm2 commented May 5, 2018 • edited Loading

All you need to know about async for this PR:

And all you need to know about SynchronizationContext:

jnm2 commented May 5, 2018

ChrisMaddock commented May 7, 2018

jcansdale commented May 8, 2018 • edited Loading

jnm2 commented May 8, 2018 • edited Loading

ChrisMaddock left a comment • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jnm2 commented May 12, 2018

jcansdale commented May 13, 2018 • edited Loading

jnm2 commented May 13, 2018 • edited Loading

jnm2 commented May 13, 2018

jcansdale left a comment

Choose a reason for hiding this comment

jcansdale commented May 14, 2018

jnm2 commented May 14, 2018

rprouse commented May 14, 2018

jnm2 commented May 14, 2018

Belorus commented May 27, 2018 • edited Loading

Belorus commented May 27, 2018 • edited Loading

CharliePoole commented May 27, 2018

Belorus commented May 27, 2018

CharliePoole commented May 27, 2018

jnm2 commented May 27, 2018

jnm2 commented May 27, 2018

jnm2 commented Apr 28, 2018 •

edited

Loading

jnm2 commented May 5, 2018 •

edited

Loading

jcansdale commented May 8, 2018 •

edited

Loading

jnm2 commented May 8, 2018 •

edited

Loading

ChrisMaddock left a comment •

edited

Loading

jcansdale commented May 13, 2018 •

edited

Loading

jnm2 commented May 13, 2018 •

edited

Loading

Belorus commented May 27, 2018 •

edited

Loading

Belorus commented May 27, 2018 •

edited

Loading