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

Exceptions in Fire&Forget tasks can be missed by our tests #46808

Open
adamsitnik opened this issue Jan 11, 2021 · 5 comments
Open

Exceptions in Fire&Forget tasks can be missed by our tests #46808

adamsitnik opened this issue Jan 11, 2021 · 5 comments
Labels
area-Meta discussion test-enhancement Improvements of test source code
Milestone

Comments

@adamsitnik
Copy link
Member

#46774 has shown, that there are some Fire&Forget tasks that can throw an unnoticed exception when we introduce a bug in BCL (see #46807 (comment) for details)

This is of course very rare as we try to avoid Fire&Forget tasks, but anyway it is possible (at least for the MemoryCache tests).

I wonder whether there is anything that we have done in the past to detect such issues?

If not, should we do something about it? Like for example extending xunit test runner to report a failure on TaskScheduler.UnobservedTaskException event? (I don't know if it's currently possible, it's just a hypothetical example to start a discussion)

[Fact]
public void ThrowingFireAndForgetThatPasses()
{
    Schedule(new object());

    static void Schedule(object obj)
    {
        Task.Factory.StartNew(state =>
        {
            string text = (string)state; // fails
            Console.WriteLine(text);
        }, obj, CancellationToken.None, TaskCreationOptions.DenyChildAttach, TaskScheduler.Default);
    }
}

@stephentoub @eerhardt

@ghost
Copy link

ghost commented Jan 11, 2021

Tagging subscribers to this area: @safern, @ViktorHofer
See info in area-owners.md if you want to be subscribed.

Issue Details

#46774 has shown, that there are some Fire&Forget tasks that can throw an unnoticed exception when we introduce a bug in BCL (see #46807 (comment) for details)

This is of course very rare as we try to avoid Fire&Forget tasks, but anyway it is possible (at least for the MemoryCache tests).

I wonder whether there is anything that we have done in the past to detect such issues?

If not, should we do something about it? Like for example extending xunit test runner to report a failure on TaskScheduler.UnobservedTaskException event? (I don't know if it's currently possible, it's just a hypothetical example to start a discussion)

[Fact]
public void ThrowingFireAndForgetThatPasses()
{
    Schedule(new object());

    static void Schedule(object obj)
    {
        Task.Factory.StartNew(state =>
        {
            string text = (string)state; // fails
            Console.WriteLine(text);
        }, obj, CancellationToken.None, TaskCreationOptions.DenyChildAttach, TaskScheduler.Default);
    }
}

@stephentoub @eerhardt

Author: adamsitnik
Assignees: -
Labels:

Discussion, area-Infrastructure-libraries, test enhancement

Milestone: -

@Dotnet-GitSync-Bot Dotnet-GitSync-Bot added the untriaged New issue has not been triaged by the area owner label Jan 11, 2021
@stephentoub
Copy link
Member

I wonder whether there is anything that we have done in the past to detect such issues?

Not really; it's up to the test to not eat exceptions. Even a TaskScheduler.UnobservedTaskException wouldn't necessarily help, as it would only trigger upon finalization happening; there are also some places where product code has done it, in some cases intentionally allowing exceptions to be eaten (for better or worse), which would make such an enabled feature problematic.

Every now and then I have done a search to ensure we don't have async void tests, but that's obviously just one manifestation of this. At the end of the day, it's not all that different from a try/catch{} that any test code could have and similarly eat all exceptions, including assert failures.

@Clockwork-Muse
Copy link
Contributor

....note that this is one of the big reasons TDD mandates writing a failing test first - if you don't know what failure looks like, you can't be sure of your success.

@ViktorHofer
Copy link
Member

Moving out of Infrastructure as this isn't specific to our test infrastructure (MSBuild, xunit aquisition, etc.)

@joperezr joperezr removed the untriaged New issue has not been triaged by the area owner label Mar 9, 2021
@joperezr joperezr added this to the Future milestone Mar 9, 2021
@IGx89
Copy link

IGx89 commented Jan 6, 2023

+1. We have an Azure App Service webapp occasionally crashing due to an unhandled exception thrown from an async void method. Easy to fix, but while attempting to follow TDD and write a failing test for that situation I've had no luck. Neither TaskScheduler.UnobservedTaskException nor AppDomain.CurrentDomain.UnhandledException catch the exception, and AppDomain.CurrentDomain.FirstChanceException does but catches all exceptions and not just unhandled.

The creator of Xunit says it's due to the VSTest engine swallowing the exception (xunit/xunit#157).

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area-Meta discussion test-enhancement Improvements of test source code
Projects
No open projects
Development

No branches or pull requests

7 participants