-
Notifications
You must be signed in to change notification settings - Fork 5.9k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Document MSTEST0039 and MSTEST0040 #44419
base: main
Are you sure you want to change the base?
Conversation
|
||
## Rule description | ||
|
||
Exceptions that are thrown in an `async void` context are unobserved. So, a failing assertion in an `async void` method may go unnoticed. When using VSTest, such exceptions are very likely to be unnoticed. When using Microsoft.Testing.Platform, such exception *may* crash the process |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@stephentoub I'll appreciate if you can review the wording for accuracy here please.
To share context: in the case of Microsoft.Testing.Platform, we do subscribe to both TaskScheduler.UnobservedTaskException
and AppDomain.CurrentDomain.UnhandledException
and call FailFast
. In VSTest, we don't subscribe.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
How do you prevent the crashing in VSTest? Is there a SynchronizationContext that's eating the exception? Are unhandled exceptions on ThreadPool threads configured to not crash?
Exceptions in an async void method are just queued to be thrown on the current SynchronizationContext if there is one, or if there isn't on the ThreadPool.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hmm.. VSTest doesn't have synchronization context. Not sure about the second question of configuring ThreadPool threads to not crash. @Evangelink @nohwnd Do you know?
@Evangelink Could it be that the test project where the original issue is noticed is setting their own synchronization context? 👀
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'll look at VSTest codebase when I'm back at laptop.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I tried a quick sample in a VSTest project. The test passes when async void
which is kinda expected even for MTP, but I can see the UnhandledException being printed with VSTest.
Maybe it's just about that FailFast in MTP which will simply cause non-zero exit code and things will be failing properly in CI contexts etc?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Original issue was on netfx so this is pretty sure it's linked.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, that's what I meant by "Are unhandled exceptions on ThreadPool threads configured to not crash?" That setting reverts to the .NET Framework 1.x behavior circa 20 years ago where the ThreadPool eats unhandled exceptions rather than allowing them to crash the process.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is the following wording more accurate then?
Exceptions that are thrown in an `async void` context are unhandled. A failing assertion in an `async void` method will be swallowed and will not crash the process when using VSTest under .NET Framework, and may crash the process when using Microsoft.Testing.Platform or VSTest under modern .NET.
(Note: edited after commenting)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Seems reasonable I'd guess. I'd find the behavior more intuitive if it behaved like xunit does: xunit registers its own SynchronizationContext in order to make async void
work, both in terms of knowing when the test completes and knowing when an exception occurs. async void
still isn't recommended, but having one doesn't put the whole process in jeopardy.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@stephentoub Interesting. Let me open an issue for that on our side.
Fixes microsoft/testfx#4691
Fixes microsoft/testfx#4690
cc @Evangelink
Internal previews
[DeploymentItem]
can be specified only on test class or test method