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

Use CancellationToken.ThrowIfCancellationRequested #33809

Closed
terrajobst opened this issue Mar 19, 2020 · 5 comments · Fixed by dotnet/roslyn-analyzers#4864
Closed

Use CancellationToken.ThrowIfCancellationRequested #33809

terrajobst opened this issue Mar 19, 2020 · 5 comments · Fixed by dotnet/roslyn-analyzers#4864
Assignees
Labels
api-approved API was approved in API review, it can be implemented area-System.Threading code-analyzer Marks an issue that suggests a Roslyn analyzer code-fixer Marks an issue that suggests a Roslyn code fixer help wanted [up-for-grabs] Good issue for external contributors
Milestone

Comments

@terrajobst
Copy link
Member

Flag code that does if (token.IsCancellationRequested) throw new OperationCanceledException(); or if (token.IsCancellationRequested) throw new OperationCanceledException(token); and replace with token.ThrowIfCancellationRequested();.

Category: Style

@terrajobst terrajobst added api-suggestion Early API idea and discussion, it is NOT ready for implementation area-System.Threading 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
@dotnet dotnet deleted a comment from Dotnet-GitSync-Bot 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
@jeffschwMSFT jeffschwMSFT added untriaged New issue has not been triaged by the area owner and removed untriaged New issue has not been triaged by the area owner labels Apr 9, 2020
@carlossanlop
Copy link
Member

carlossanlop commented Jan 15, 2021

Suggested severity: Info

void MyMethod(CancellationToken c)
{
    // A) Simple check
    // Before
    if (c.IsCancellationRequested)
    {
        // Not sure if we want to cover other overloads as we cannot pass the parameters
        throw new OperationCanceledException();
    }

    // After
    c.ThrowIfCancellationRequested();

    // A) Negated check
    // Before
    if (!c.IsCancellationRequested)
    {
        DoSomething();
    }
    else
    {
        throw new OperationCanceledException();
    }

    // After
    c.ThrowIfCancellationRequested();
    DoSomething();


    // Excluded cases:

    // B) Multiple conditions in the if statement
    if (c.IsCancellationRequested && otherCondition)
    {
        throw new OperationCanceledException();
    }

    // C) There is extra logic in the if body
    if (c.IsCancellationRequested)
    {
        SomeOtherCode();
        throw new OperationCanceledException();
    }
}

@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 Jan 15, 2021
@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

  • Looks good
  • We should only suggest this for OperationCanceledException, not TaskCanceledException because ThrowIfCancellationRequested throws the former.

@buyaa-n buyaa-n added the help wanted [up-for-grabs] Good issue for external contributors label Feb 4, 2021
@NewellClark
Copy link
Contributor

I would like to be assigned to this.

@stephentoub
Copy link
Member

This was implemented as CA2250. 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 code-analyzer Marks an issue that suggests a Roslyn analyzer code-fixer Marks an issue that suggests a Roslyn code fixer help wanted [up-for-grabs] Good issue for external contributors
Projects
None yet
Development

Successfully merging a pull request may close this issue.

8 participants