All calls to SourceRepository.GetResource must pass a cancellation token #5631
Conversation
| @@ -135,6 +136,7 @@ public virtual async Task<FeedType> GetFeedType(CancellationToken token) | |||
| /// </summary> | |||
| /// <typeparam name="T">Expected resource type</typeparam> | |||
| /// <returns>Null if the resource does not exist</returns> | |||
| [Obsolete("Use the overload that takes a CancellationToken. If you don't want to support cancelation, use CancellationToken.None.")] | |||
There was a problem hiding this comment.
As mentioned in the PR description, this is effectively a breaking change in our public APIs. And it's a commonly used API, so this will be "highly" impactful. However, the fix is trivial, as the message explains.
I think it's worthwhile to make this breaking change, as it's a long term win, minimizing risk of bugs similar to the two linked to this PR. Please reply if you don't think we should mark these APIs as obsolete, so as not to impact customers using the APIs in their own apps.
| @@ -34,7 +35,7 @@ public static class DeleteRunner | |||
| { | |||
| logger.LogWarning(string.Format(CultureInfo.CurrentCulture, Strings.Warning_HttpServerUsage, "delete", packageSource.Source)); | |||
| } | |||
| var packageUpdateResource = await CommandRunnerUtility.GetPackageUpdateResource(sourceProvider, packageSource); | |||
| var packageUpdateResource = await CommandRunnerUtility.GetPackageUpdateResource(sourceProvider, packageSource, CancellationToken.None); | |||
There was a problem hiding this comment.
Currently delete is only available in CLI tools, and we don't handle ctrl-c in any of them, so technically the lack of cancellation passthough won't affect NuGet tooling, but if customers are using NuGet.Commands in their own apps, there's no way for them to cancel this request.
| @@ -58,7 +59,7 @@ public static class PushRunner | |||
| && !sourceUri.IsFile | |||
| && sourceUri.IsAbsoluteUri) | |||
| { | |||
| symbolPackageUpdateResource = await CommandRunnerUtility.GetSymbolPackageUpdateResource(sourceProvider, source); | |||
| symbolPackageUpdateResource = await CommandRunnerUtility.GetSymbolPackageUpdateResource(sourceProvider, source, CancellationToken.None); | |||
There was a problem hiding this comment.
Similar to delete, currently push is only available though NuGet's CLI tools, where we don't handle ctrl-c ourselves, so the .NET runtime will kill the process immediately. However, customers calling this method in their own apps won't be able to cancel the push.
| @@ -435,7 +435,7 @@ public static HttpSource Create(SourceRepository source, IThrottle throttle) | |||
| throw new ArgumentNullException(nameof(throttle)); | |||
| } | |||
|
|
|||
| Func<Task<HttpHandlerResource>> factory = () => source.GetResourceAsync<HttpHandlerResource>(); | |||
| Func<Task<HttpHandlerResource>> factory = () => source.GetResourceAsync<HttpHandlerResource>(CancellationToken.None); | |||
There was a problem hiding this comment.
While this is in a Client SDK package, anything that wants to make an HTTP request has to get the HttpHandlerResource, hence we can be confident that the HttpHandlerResourceProvider isn't going to be making HTTP requests (or do any other async work), so using CancellationToken.None here won't cause any blocking on cancellation requested.
There was a problem hiding this comment.
We can probably do this change without obsoleting the methods.
For example, we can ban the usage of those methods using https://github.com/NuGet/NuGet.Client/blob/dev/build/BannedSymbols.txt.
Just an option.
src/NuGet.Clients/NuGet.PackageManagement.PowerShellCmdlets/NuGetPowerShellBaseCommand.cs
Show resolved
Hide resolved
zivkan
dismissed stale reviews from nkolev92, donnie-msft, and jeffkl
via
15d0b55
Bug
Fixes: NuGet/Home#13234
Fixes: NuGet/Home#11813
Regression? No
Description
CancellationTokens weren't being passed around everywhere needed. In the case of the two linked issues, it wasI don't know what NuGet.Protocol looked like before the V3 protocol was added, but since it was added, there are resource providers that need the service index, so it no longer makes sense to call
GetResourceAsyncorGetResourcewithout passing a cancellation token. Therefore I marked these two overloads (without the cancellation token parameter) as obsolete. This is a breaking change from an API point of view. What's worse is that these are commonly used APIs by anyone using the NuGet.Protocol package (part of the NuGet Client SDK), therefore I'm expecting high impact. However, the fix is trivial to implement.All the other changes in PR are just fixing compile errors (since we treat warnings as errors). I passed in cancellation tokens where possible, but there are some methods that don't have a cancellation token already, but mostly this was in NuGet.CommandLine or NuGet.CommandLine.XPlat, where we don't handle ctrl-c directly anyway.
Honestly, it would be great to ban
CancellationToken.Nonein our non-test code, but it will require a bunch of breaking API changes where existing public APIs need to start taking in a cancellation token, which is more effort than I'm willing to make for this quick fix.PR Checklist
PR has a meaningful title
PR has a linked issue.
Described changes
Tests
Documentation