Skip to content
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

Open
wants to merge 3 commits into
base: main
Choose a base branch
from

Conversation

@Youssef1313 Youssef1313 requested review from IEvangelist and a team as code owners January 17, 2025 18:03
@dotnetrepoman dotnetrepoman bot added this to the January 2025 milestone Jan 17, 2025
@dotnet-policy-service dotnet-policy-service bot added dotnet-fundamentals/svc community-contribution Indicates PR is created by someone from the .NET community. labels Jan 17, 2025

## 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
Copy link
Member Author

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.

Copy link
Member

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.

Copy link
Member Author

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? 👀

Copy link
Member

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.

Copy link
Member Author

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?

Copy link
Member

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.

Copy link
Member

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.

Copy link
Member Author

@Youssef1313 Youssef1313 Jan 17, 2025

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)

Copy link
Member

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.

Copy link
Member Author

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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
community-contribution Indicates PR is created by someone from the .NET community. dotnet-fundamentals/svc
Projects
None yet
3 participants