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

Analyzer proposal: Detect wrong CancellationToken being used when multiple tokens are available in scope #88403

Open
mavasani opened this issue Jul 5, 2023 · 1 comment
Labels
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 feature-request
Milestone

Comments

@mavasani
Copy link

mavasani commented Jul 5, 2023

Issue #33774 was used to implement the rule CA2016 (Forward the CancellationToken parameter to methods that take one), via PR dotnet/roslyn-analyzers#3641. This rule flags invocations where cancellation token wasn't being passed, when one was available in the context of the invocation.

I'd like to propose either an extension to this rule or a new rule to detect when the wrong cancellation token is being passed, when multiple tokens are available in scope.

  1. Pattern 1: For example, an async method that takes a cancellation token parameter, say ct1, defines a lambda or a local function that takes another cancellation token parameter, say ct2, and an invocation within this lambda or local function uses ct1 instead of ct2. This inevitably indicates a functional bug. We were bitten by this very recently in Roslyn - see Thread in the correct cancellation token into the code action operation roslyn#68859, and using the wrong cancellation token in a code fixer led to a very subtle, hard to diagnose bug.

  2. Pattern 2: Another pattern that we may want to consider to detect is when a type defines a field of type CancellationToken and also has an async method that takes a CancellationToken parameter, and an invocation within this method uses the cancellation token field instead of the parameter. This pattern may or may not indicate a bug, but still definitely indicates code smell and bad design - it should be recommended to either always have CancellationToken be a method parameter OR a field, but not both within the same type.

Proposed category: Reliability
Proposed severity: IDE suggestion
Code fixer: Should be trivial to implement

@mavasani mavasani added area-System.Threading feature-request code-analyzer Marks an issue that suggests a Roslyn analyzer labels Jul 5, 2023
@ghost ghost added the untriaged New issue has not been triaged by the area owner label Jul 5, 2023
@ghost
Copy link

ghost commented Jul 5, 2023

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

Issue Details

Issue #33774 was used to implement the rule CA2016(Forward the CancellationToken parameter to methods that take one), via PR dotnet/roslyn-analyzers#3641. This rule flags invocations where cancellation token wasn't being passed, when one was available in the context of the invocation.

I'd like to propose either an extension to this rule or a new rule to detect when the wrong cancellation token is being passed. For example, an async method that takes a cancellation token parameter, say ct1, defines a lambda or a local function that takes another cancellation token parameter, say ct2, and an invocation within this lambda or local function uses ct1 instead of ct2. This inevitably indicates a functional bug. We were bitten by this very recently in Roslyn - see dotnet/roslyn#68859, and using the wrong cancellation token in a code fixer led a very subtle, hard to diagnose bug.

Another pattern that we may want to consider to detect is when a type defines a field of type CancellationToken and also has an async method that takes a CancellationToken parameter, and an invocation within this method uses the cancellation token field instead of the parameter. This pattern may or may not indicate a bug, but still definitely indicates code smell and bad design - it should be recommended to either always have CancellationToken be a method parameter OR a field, but not both within the same type.

Proposed category: Reliability
Proposed severity: IDE suggestion
Code fixer: Should be trivial to implement

Author: mavasani
Assignees: -
Labels:

area-System.Threading, feature-request, code-analyzer

Milestone: -

@mangod9 mangod9 removed the untriaged New issue has not been triaged by the area owner label Jul 20, 2023
@mangod9 mangod9 added this to the Future milestone Jul 20, 2023
@buyaa-n buyaa-n added the api-suggestion Early API idea and discussion, it is NOT ready for implementation label Mar 12, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
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 feature-request
Projects
None yet
Development

No branches or pull requests

3 participants