Runtime: Forward CancellationToken to methods that can take it

[terrajobst]

The rule should try to identify places where a CancellationToken should have been passed but wasn't, e.g. in an async method that takes a CancellationToken, a method is called that has an overload that takes a CancellationToken but a shorter overload that doesn't take a CancellationToken was used instead.

Category: Reliability

[stephentoub]

cc: @marklio

[jeffhandley]

Estimates:

• Analyzer: Small
• Fixer: Medium

[stephentoub]

Analyzer: Small

The challenge here is around ensuring we invest enough thought in the design to minimize false positives. It might be more than "small".

Fixer: Medium

Conversely, I would guesstimate that once we've identified the problem (e.g. method XYZ(arg1, arg2, arg3) is being called, you have a cancellation token, and there's an XYZ(arg1, arg2, arg3, cancellation token) overload you could be using instead), fixing it will be fairly straightforward.

[bartonjs]

Analyzer heuristic:

• Only applies to methods using the async state machine generator (async/await) that take a CancellationToken value.
• For all invocation operations, if the invocation is already passing a CancellationToken, move to the next invocation.
• Otherwise, if the method is being invoked with an implicit default CancellationToken, or there is an overload to the method that takes a CancellationToken, report a warning against the method invocation (e.g. squiggle under the word "PostAsync" in a call like "client.PostAsync(data)`).

Fixer:

• No fixer if the current async method takes more than one CancellationToken.
• When the target method already takes a CancellationToken, but is invoked with a default parameter, specify the parameter (using the named parameter syntax if required).
• When an overload has the CancellationToken in a "one more parameter" or "one named parameter" compatible call, pass the declaring method's CancellationToken.
• If there is no "call compatible" overload, no fixer.

[marklio]

@bartonjs I would also be wary of fixers in the following "harder to detect" scenarios:

• The method makes use of a CancellationTokenSource
• The method registers callbacks on a token - We had some tricky finds in the prototype we built where passing a default token was the right thing to do because the cancellation strategy relied on the callback to keep state uncorrupted. Careful review was required to recognize this. Stephen saw it and I didn't.

[AArnott]

This is redundant with https://github.com/microsoft/vs-threading/issues/591, which is likely to get funded soon.
The vs-threading analyzers target less frameworks than .NET 5. Can we consolidate rules for this one on vs-threading so that folks can get the benefit without targeting .NET 5?

Or if your plan is to target all TFMs with these analyzers, I can close ours as redundant with yours.

[stephentoub]

The vs-threading analyzers target less frameworks than .NET 5. Can we consolidate rules for this one on vs-threading so that folks can get the benefit without targeting .NET 5?

There is no .NET 5 requirement. The rules are written to look for the presence of types/methods, not a specific version.

[AArnott]

Good to hear. Note we also proposed a related rule to encourage async methods to take a CancellationToken parameter in the first place: https://github.com/microsoft/vs-threading/issues/394

Regarding our https://github.com/microsoft/vs-threading/issues/591 proposal which is largely redundant with this one, we were not going to limit it to only async methods. I don't know why we would want the behavior talked about here that would only impact async methods. If a sync method takes a CancellationToken, it should propagate it wherever possible as well.

Per my blog post on the subject a method may at some point transition to a point where cancellation should no longer apply (it's past the "point of no cancellation") because it can't clean up properly if cancellation were honored.
In discussing this topic with @sharwell we were trying to figure out how to appease an analyzer so that it wouldn't flag invocations beyond that point. We agreed that this analyzer should still require CancellationToken be passed everywhere, but at the point where the method should no longer be canceled, a cancellationToken = default; statement can be written. It's very clear, gives a great place to add a code comment too, and allows all code to follow the safe pattern of propagating tokens.

Thoughts?

[terrajobst]

@AArnott

Can we consolidate rules for this one on vs-threading so that folks can get the benefit without targeting .NET 5?

My opinion is that analyzers for core BCL APIs should be in-box. And shipping the VS analyzers in-box doesn't make a lot of sense. As Stephen said, the analyzers themselves don't check for specific frameworks. However, you need to build with SDK-style projects in order to get the benefit of these analyzers (and have the .NET 5 SDK installed).

[AArnott]

The .NET 5 SDK requirement is unfortunate, and means that the very earliest people can use these analyzers in a stable form in November, right?

And shipping the VS analyzers in-box doesn't make a lot of sense.

I'd like to discuss this further. We actually already have a backlog item on the fxcop analyzer package to add a dependency on the vs-threading analyzers package. There's nothing VS-specific about them, nor ever will be. We have a separate analyzer package for VS-specific concerns.
We have a lot of value in the existing vs-threading analyzers that go well beyond this issue or any other issue I see on your backlog.
I'd like to avoid multiple analyzers that verify the same thing, both to avoid customer frustration with suppressing multiple analyzers or seeing duplicate warnings, as well as to avoid wasted effort across teams in developing similar rules.

Also, moving analyzers from vs-threading to roslyn-analyzers has a couple of major downsides:

1. the rule ID would presumably change, along with our AdditionalFiles naming scheme, which would be disruptive to our existing users.
2. it wouldn't be available downlevel where it exists today.

After discussions with the Roslyn team (@jinujoseph in particular) we came to the conclusion that the best thing would be to add a package reference to the vs-threading analyzers package from Microsoft.CodeAnalysis.FxCopAnalyzers.

With that in mind then, does that change how you feel about any of this?

[stephentoub]

The .NET 5 SDK requirement is unfortunate, and means that the very earliest people can use these analyzers in a stable form in November, right?

This is not a requirement, at least not in any of the conversations I've been a part of. There is still going to be a nuget package, just as there is and has been. Rules that have recently been written are already showing up in the already published nuget package. @mavasani, has this changed?

I'd like to avoid multiple analyzers that verify the same thing

This is already the case, e.g. both packages have a rule that validates tasks are awaited. Why does vs-analyzers have its own when roslyn-analyzers already has one?

we were trying to figure out how to appease an analyzer so that it wouldn't flag invocations beyond that point

If a method accepts a CancellationToken, and the caller passes in default / CancellationToken.None, I wouldn't want the analyzer to warn. Explicitly passing in default/None is a clear indication of "I'm choosing to propagate something other than the original token". So, to me, I don't see the benefit of that cancellationToken = default; thing: if a method has a CancellationToken argument in an overload or in a default parameter where the call site isn't passing one in, the analyzer would warn, and the developer would silence it by passing in default/None. I actually see the cancellationToken = default; thing as leading to more problems. Consider a made-up method like the following, which exemplifies some patterns that actually show up:
C# public void FinishWork<T>( TaskCompletionSource<T> tcs, Func<CancellationToken, Task<T>> func, CancellationToken cancellationToken) { Task.Run(async () => { try { tcs.SetResult(await func(cancellationToken)); } catch (Exception e) { tcs.SetException(e); } }); }
the analyzer is going to warn about not flowing a cancellation token as an argument to Task.Run, but "fixing" that by putting cancellationToken = default; above the Task.Run would break the functionality, as would flowing the real token into Task.Run's argument... the right "fix" here (if a change is to be made) is to just add CancellationToken.None as the CancellationToken argument to Task.Run to silence the warning.

I don't know why we would want the behavior talked about here that would only impact async methods.

I'm fine with this validating all methods that take a cancellation token. I don't remember where the cited constraint came from.

[terrajobst]

@AArnott

The .NET 5 SDK requirement is unfortunate, and means that the very _earliest_ people can use these analyzers in a stable form in November, right?

As @stephentoub said, it looks like I was likely wrong and the analyzers we'll ship in a standalone NuGet package.

And shipping the VS analyzers in-box doesn't make a lot of sense.

I'd like to discuss this further. We actually already have a backlog item on the fxcop analyzer package to add a dependency on the vs-threading analyzers package. There's nothing VS-specific about them, nor ever will be. We have a separate analyzer package for VS-specific concerns.

I totally agree with you that we shouldn't be shipping multiple analyzers and fixers for the same area. However, this is the first release where the BCL team is seriously looking into analyzers, so some overlap is unavoidable.

But my overarching concern is long term consistency. Moving functionality from higher layers to lower layers almost always results in changes to design and/or naming. We can't meaningfully evolve the BCL when we're asked to preserve naming conventions from higher layers because that will quickly create a mess. To me, analyzers are in the exact same category. If I write a web site on Linux it makes zero sense to see analyzers with any VS naming in them. They should feel and behave like any other analyzer tackling BCL APIs. If we do preserve naming and shipping vehicles, then we're shipping the org chart.

It seems to me the right approach is combining forces across all our teams that worked on analyzers and moving them where they belong. Yes, there is some pain with shuffling things around, but long term we're all better off.

[AArnott]

This is already the case, e.g. both packages have a rule that validates tasks are awaited. Why does vs-analyzers have its own when roslyn-analyzers already has one?

I'm not denying that redundancy already exists.
When we wrote VSTHRD110 (I don't know if the condition is still true) the Roslyn analyzers only fired within async methods. So if a non-async method invoked a Task returning method and ignored the result, no warning would be produced. It turns out that hid a lot of bugs, so we wrote our own analyzer to fill in the gap.

And for the benefit of the wider audience, here is our full list of existing vs-threading analyzers.

If a method accepts a CancellationToken, and the caller passes in default / CancellationToken.None, I wouldn't want the analyzer to warn. Explicitly passing in default/None is a clear indication of "I'm choosing to propagate something other than the original token"

We considered that argument, but dismissed it because many methods exist that require a CancellationToken parameter, so a caller would explicitly pass in a default token there if it didn't have one handy. But later when the calling method's signature changes to add a CancellationToken, all the places where default was passed should now be updated to take the token, but if we followed your proposed rule nothing would be flagged, leaving bugs in a large method where the token wasn't always passed in.

It also doesn't set a clear pattern for 'the point of no cancellation'. If I intentionally start passing default in halfway through the method, a reader might think I forgot to update these places and replace default with cancellationToken and actually introduce a bug. But if instead I use cancellationToken everywhere in the method, but halfway through I reset that local variable to default, it's very clear what the intent was.

Your counter example on Task.Run is a good one though.

I'm fine with this validating all methods that take a cancellation token. I don't remember where the cited constraint came from.

The issue description itself: "e.g. in an async method that takes a CancellationToken". And again in @bartonjs's mini-spec further down where it's made much more explicit "Only applies to methods using the async state machine generator".

[mavasani]

We actually already have a backlog item on the fxcop analyzer package to add a dependency on the vs-threading analyzers package.

@AArnott Since we last talked on those items, FxCop analyzers has taken a backseat and will be superceded by this new analyzer package. Majority of ported legacy FxCop CA analyzers will be disabled by default or switched to an IDE-only suggestions, and new analyzers written by CoreFx team will be enabled by default. We should think the path forward for VS threading analyzers - should these be merged into this same package? Should these continue to exist in their current repo, but just follow the same insertion model as new SDK analyzer assembly and directly install with .NET5 SDK? It might be good to setup a meeting to decide the path forward here.

[AArnott]

@mavasani @terrajobst I like the idea of a meeting to get consensus here. I can see myself supporting a migration of vs-threading analyzers to ... wherever this new home is. But I'd like to understand better what's changing, how existing users can continue to use the analyzers in their new home, and (the hardest one) which of our vs-threading analyzers to move. We've had such meetings at least twice before, and each time the work ended up getting cut to move, copy, or recreate the analyzers.

[stephentoub]

so we wrote our own analyzer to fill in the gap.

FWIW, I'd prefer the default reaction to be "let's fix the existing analyzer" rather than "let's build a duplicative analyzer".

if we followed your proposed rule nothing would be flagged, leaving bugs in a large method where the token wasn't always passed in.

Yup. I still believe explicitly passing in default is the better route. Setting cancellationToken = default; has the issues I outlined, and is generally an unnatural gesture. If code wants to do that and continue to pass in cancellationToken, that's fine, but I strongly believe the analyzer should not warn if the caller is passing in a token, regardless of that token's value.

I don't remember where the cited constraint came from.

The issue description itself

No, I mean, I don't remember why the issue says that.

[AArnott]

FWIW, I'd prefer the default reaction to be "let's fix the existing analyzer" rather than "let's build a duplicative analyzer".

I like that. But when we did this, it wasn't an analyzer. It was a built-in compiler warning. And the compiler team made an intentional choice to not warn in that case because they wanted upgrading compiler versions to not produce a bunch of new warnings in existing code.

but I strongly believe the analyzer should not warn if the caller is passing in a token, regardless of that token's value.

That's fine. I'm flexible on that point. I just wanted to share what research and thought had already gone into the area.

[stephentoub]

I like that.

Great.

But when we did this, it wasn't an analyzer. It was a built-in compiler warning. And the compiler team made an intentional choice to not warn in that case because they wanted upgrading compiler versions to not produce a bunch of new warnings in existing code.

Sorry, I was typing too quickly earlier... I meant to say "awaited without a ConfigureAwait". That's the functionality I was referring to. That's always been an analyzer (or previously IL-based FxCop rule) rather than a compiler warning.

[bartonjs]

I wrote it down because it got said on the call :smile:. I think it was just trying to limit work; but if it's already been shown to not be too much work, then the more the merrier.

[AArnott]

I meant to say "awaited without a ConfigureAwait".

Ah, yes. And if/when we consolidate, I imagine our version of that analyzer will just disappear, assuming a comparison shows ours doesn't add anything unique any more.

[stephentoub]

I think it was just trying to limit work

Work (CPU) done by the analyzer you mean? (As opposed to work done by a developer implementing the analyzer)

[stephentoub]

assuming a comparison shows ours doesn't add anything unique any more.

If it does, we should consolidate, rather than have two :smile:

[bartonjs]

Yeah, the CPU-work of finding "compatible" alternatives that take a CancellationToken. (Implicitly using a default value seems pretty mild) ... at least, if it's a rule that we expect to be on by default during CLI compilations.

Though it's probably enough noise that we wouldn't even turn it on by default with the "waves".

[carlossanlop]

The issue has been assigned to me, but I'd like to know if we have green light to start working on this.

From the above discussion it's not entirely clear to me what course should we take:

• Should I move an existing microsoft/vs-threading analyzer to dotnet/roslyn-analyzers and improve it?
• Should I write one from scratch in dotnet/roslyn-analyzers
• Or something else?

Also, regarding these comments:

I meant to say "awaited without a ConfigureAwait". That's the functionality I was referring to. That's always been an analyzer (or previously IL-based FxCop rule) rather than a compiler warning.

I think it was just trying to limit work; but if it's already been shown to not be too much work, then the more the merrier.

It may be helpful/relevant to mention that I recently merged a new Roslyn analyzer that looks for any Stream.ReadAsync or Stream.WriteAsync invocations and suggests the better, newer overload with slightly different arguments. The analyzer will only generate a diagnostic when the invocation is awaited, and it also works if the user calls ConfigureAwait. I'm currently working on the fixer, which will be capable of detecting the CancellationToken argument if the user passed one. Here's the issue, the analyzer PR and the fixer PR, for anyone curious.

[stephentoub]

Should I write one from scratch in dotnet/roslyn-analyzers

Yes. (There isn't one implemented in vs-threading, just the idea for one.)

Or something else?

Please sync with @marklio. He has a prototype implementation of one. It may not be exactly the direction we want to go, but it might help seed some ideas.

It may be helpful

Thanks. I suggest we start with something along the lines of the following:

• For methods that take one CancellationToken argument, optional or not.
• Find all method calls in its body.
• Ignore any that take a CancellationToken parameter to which something is already being passed explicitly, e.g. don't warn if the supplied token, default, default(CancellationToken), CancellationToken.None, or anything else is being explicitly passed....
• Warn on any that take an optional CancellationToken parameter where that parameter has been omitted, e.g. calling stream.WriteAsync(readOnlyMemory), as that has an optional CancellationToken parameter that was omitted.
• Warn on any that have an overload that takes the exact same parameters as are currently being passed plus a CancellationToken parameter, e.g. if stream.WriteAsync(array, 0, array.Length) is called, it should warn because there's a stream.WriteAsync(byte[], int, int, CancellationToken) overload.

We may need to tweak this a bit as we go, but my expectation is this will lead to a reasonable balance between false positives and false negatives.

[carlossanlop]

_Please sync with @marklio. He has a prototype implementation of one. It may not be exactly the direction we want to go, but it might help seed some ideas._

I synced with Mark and he shared the prototype with me. I'm taking a look to determine how much of the code can be reused for this analyzer.

[stephentoub]

Thanks.

[carlossanlop]

The code has been merged. Reopening because the documentation PR is still being reviewed.

[carlossanlop]

This roslyn analyzer has been merged as well as its documentation, so we can consider it fixed.
Only pending task is to merge the PR that applies this analyzer's findings in the runtime repo, which is being tracked here: https://github.com/dotnet/runtime/pull/37607