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

Do not call Task.WaitAll with a single argument #33807

Closed
terrajobst opened this issue Mar 19, 2020 · 7 comments
Closed

Do not call Task.WaitAll with a single argument #33807

terrajobst opened this issue Mar 19, 2020 · 7 comments
Assignees
Labels
api-approved API was approved in API review, it can be implemented area-System.Threading.Tasks code-analyzer Marks an issue that suggests a Roslyn analyzer code-fixer Marks an issue that suggests a Roslyn code fixer
Milestone

Comments

@terrajobst
Copy link
Member

Task.Wait can be used instead.

Category: Performance

@terrajobst terrajobst added api-suggestion Early API idea and discussion, it is NOT ready for implementation area-System.Threading.Tasks code-analyzer Marks an issue that suggests a Roslyn analyzer labels Mar 19, 2020
@Dotnet-GitSync-Bot Dotnet-GitSync-Bot added the untriaged New issue has not been triaged by the area owner label Mar 19, 2020
@jeffhandley jeffhandley added this to the Future milestone Mar 20, 2020
@jeffhandley jeffhandley added the code-fixer Marks an issue that suggests a Roslyn code fixer label Mar 21, 2020
@jeffhandley
Copy link
Member

Estimates:

  • Analyzer: Small
  • Fixer: Small

@terrajobst terrajobst removed the untriaged New issue has not been triaged by the area owner label Mar 21, 2020
@Mrnikbobjeff
Copy link

I have a working analyzer for this as well, if this is ready to implement. #33806 is very similar, my analyzer would offer a great basis for that issue.

@carlossanlop
Copy link
Member

carlossanlop commented Oct 30, 2020

Here is a usage example:

using System.Threading.Tasks;

namespace MyNamespace
{
    class Sample
    {
        public static void Main()
        {
            Task task = ...;

            // Instead of this
            Task.WaitAll(task);

            // Suggest this
            task.Wait();

            
            // Instead of this
            Task.WaitAll(GetTask());

            // Suggest this
            GetTask().Wait();
        }

        static Task GetTask()
        {
            return ...;
        }
    }
}
  • This analyzer should not affect any of the other WaitAll overloads, because they take a Task[]. Only this overload, which takes a params Task[] task would be affected.
  • This analyzer should also have a fixer.
  • Suggested severity: Info.
  • Suggested milestone if approved: 6.0 (seems simple to address).

Questions

@carlossanlop carlossanlop added api-ready-for-review API is ready for review, it is NOT ready for implementation and removed api-suggestion Early API idea and discussion, it is NOT ready for implementation labels Oct 30, 2020
@terrajobst terrajobst added api-approved API was approved in API review, it can be implemented and removed api-ready-for-review API is ready for review, it is NOT ready for implementation labels Feb 4, 2021
@terrajobst
Copy link
Member Author

terrajobst commented Feb 4, 2021

Video

  • Makes sense
void M(Task task)
{
    // Before
    Task.WaitAll(task);

    // After
    task.Wait();
}

@ryzngard
Copy link

ryzngard commented Feb 5, 2021

I'll take this and #33806

@carlossanlop carlossanlop removed the help wanted [up-for-grabs] Good issue for external contributors label Feb 5, 2021
@carlossanlop
Copy link
Member

It's yours, @ryzngard. Thanks for your help! Let us know if you have any questions.

@stephentoub
Copy link
Member

This was implemented as CA1842/1843. Thanks!

@ghost ghost locked as resolved and limited conversation to collaborators Aug 12, 2021
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
api-approved API was approved in API review, it can be implemented area-System.Threading.Tasks code-analyzer Marks an issue that suggests a Roslyn analyzer code-fixer Marks an issue that suggests a Roslyn code fixer
Projects
None yet
Development

No branches or pull requests

8 participants