Conversation
| **API Pattern Discovery:** | ||
| - Study existing services (e.g., Sql, Postgres, Redis) to understand resource access patterns | ||
| - Use resource collections correctly | ||
| - ✅ Good: `.GetSqlServers().GetAsync(serverName)` |
There was a problem hiding this comment.
GetAsync here should be using the CancellationToken parameter or using .WithCancellation
| .GetLatestVirtualMachineScaleSetRollingUpgradeAsync(cancellationToken); | ||
|
|
||
| // ✅ Correct: VMSS instances | ||
| var vms = vmssResource.Value.GetVirtualMachineScaleSetVms().GetAllAsync(); |
There was a problem hiding this comment.
Missing use of Cancellationoken. Also, it's missing await on the result.
| // ✅ Correct: VMSS instances | ||
| var vms = vmssResource.Value.GetVirtualMachineScaleSetVms().GetAllAsync(); | ||
|
|
||
| // Pattern: Get{ResourceType}() returns collection, then .GetAsync() or .GetAllAsync() |
There was a problem hiding this comment.
Both GetAsync and GetAllAsync have CancellationToken arguments. This should be updated to reflect that. I think GetAsync typically takes a string argument for a resource name, so make sure your update includes that detail.
| string subscription, | ||
| string? resourceGroup = null, | ||
| RetryPolicyOptions? retryPolicy = null, | ||
| CancellationToken cancellationToken); |
There was a problem hiding this comment.
This should have an optional parameter. Probably a miss from a previous change I made.
| - **Pattern**: | ||
| ```csharp | ||
| // Correct - use service | ||
| var subscriptionResource = await _subscriptionService.GetSubscription(subscription, null, retryPolicy); |
There was a problem hiding this comment.
This line is missing using CancellationToken.
| **Issue: `cannot convert from 'System.Threading.CancellationToken' to 'string'`** | ||
| - **Cause**: Wrong parameter order in resource manager method calls | ||
| - **Solution**: Check method signatures; many Azure SDK methods don't take CancellationToken as second parameter | ||
| - **Fix**: Use `.GetAsync(resourceName)` instead of `.GetAsync(resourceName, cancellationToken)` |
There was a problem hiding this comment.
Please verify if this "issue" correct, and if the fix should also say to use .WithCancellation
|
|
||
| **Issue: Wrong resource access pattern** | ||
| - **Problem**: Using `.GetSqlServerAsync(name, cancellationToken)` | ||
| - **Solution**: Use resource collections: `.GetSqlServers().GetAsync(name)` |
| _storageService = storageService; | ||
| } | ||
|
|
||
| public override async Task<CommandResponse> ExecuteAsync( |
There was a problem hiding this comment.
I missed this in my earlier changes. ExecuteAsync has a CancellationToken argument and should be in this example. Also update the GetStorageAccountsAsync invocation as necessary to show using the CancellationToken
| CancellationToken cancellationToken = default) | ||
| { | ||
| // ✅ Use base class methods that handle authentication and ARM client creation | ||
| var armClient = await CreateArmClientAsync(tenant: null, retryPolicy); |
There was a problem hiding this comment.
CreateArmClientAsync takes a CancellationToken
| _sqlService = sqlService; | ||
| } | ||
|
|
||
| public override async Task<CommandResponse> ExecuteAsync( |
There was a problem hiding this comment.
ExecuteAsync takes a CancellationToken
| var options = BindOptions(parseResult); | ||
|
|
||
| // ✅ Service calls are async and don't store request state | ||
| var databases = await _sqlService.ListDatabasesAsync( |
There was a problem hiding this comment.
Forward CancellationToken parameter from ExecuteAsync signature.
|
|
||
| ### Azure SDK Integration | ||
| - [ ] All Azure SDK property names verified and correct | ||
| - [ ] Resource access patterns use collections (e.g., `.GetSqlServers().GetAsync()`) |
There was a problem hiding this comment.
Make sure this includes something about CancellationToken
There was a problem hiding this comment.
Potentially refer to the specific section headers with details?
| try | ||
| { | ||
| // Perform a lightweight operation to validate the client | ||
| await client.ReadAccountAsync(); |
There was a problem hiding this comment.
Follow up item: ReadAccountAsync doesn't have a signature for a CancellationToken. We'll need to use a pattern that allows us to bail out if the cancellationToken is cancelled even if the underlying operation can't be aborted.
| try | ||
| { | ||
| // Get all cached client keys | ||
| keys = await _cacheService.GetGroupKeysAsync(CacheGroup, CancellationToken.None); |
There was a problem hiding this comment.
Please create a follow up work item to make sure we remove uses of CancellationToken.None, like this line. It'll be hard somewhere like here, but we can look online to see if there are recommendations. It's possible that we just don't await if we definitively know this is happening during application exiting. For example, we might be able to DI IHostApplicationLifetime and look at it the CancellationTokens attached to it.
| { | ||
| try | ||
| { | ||
| var client = await _cacheService.GetAsync<CosmosClient>(CacheGroup, key); |
There was a problem hiding this comment.
Similar as above. We might want to investigate if we should actually await on this. I'd recommend looking online for patterns about how to handle IAsyncDisposable, deciding if we care or how we write this code (which will likely vary based on the object's DI service lifetime, e.g., singleton, scoped, transient). I don't know the best pattern off the top of my head.
| try | ||
| { | ||
| // Create ArmClient for deployments | ||
| ArmClient armClient = await CreateArmClientWithApiVersionAsync("Microsoft.CognitiveServices/accounts/deployments", "2025-06-01", null, retryPolicy); |
There was a problem hiding this comment.
Looks like CreateArmClientWithApiVersionAsync needs to add a parameter for it.
| } | ||
| }; | ||
|
|
||
| var result = await CreateOrUpdateGenericResourceAsync<Models.CognitiveServicesAccountDeploymentData>( |
There was a problem hiding this comment.
CreateOrUpdateGenericResourceAsync needs to add a CancellationToken parameter, and this invocation needs to pass one along.
| var credential = await GetCredential(tenant, cancellationToken); | ||
| var loadTestClient = new LoadTestRunClient(new Uri($"https://{dataPlaneUri}"), credential, CreateLoadTestingClientOptions(retryPolicy)); | ||
|
|
||
| var loadTestRunResponse = await loadTestClient.GetTestRunAsync(testRunId); |
There was a problem hiding this comment.
Lacks a way to be cancelled. Look into some sort of wrapping. Also, with stuff like this that are lacking CancellationToken within Azure SDK libraries, please file issues for the SDK to add them to methods that lack CancellationToken. File separate issues per resource provider for easier tracking and distribution of work by relevant owners.
There was a problem hiding this comment.
Actually, for GetTestRunAsync, it has a parameter RequestContext which has a CancellationToken property. Should we use that? If so, are there other Azure SDK methods that lack CancellationToken as explicit method parameters but do honor a RequestContext.CancellationToken? You might want to chat with someone on the .NET Azure SDK to confirm if we should depend on RequestContext.
|
|
||
| using var requestContent = RequestContent.Create(JsonSerializer.Serialize(requestBody, LoadTestJsonContext.Default.TestRunRequest)); | ||
|
|
||
| var loadTestRunResponse = await loadTestClient.BeginTestRunAsync( |
There was a problem hiding this comment.
Missing CancellationToken. As with other LoadTestingService.cs comment of mine, should we use the RequestContext parameter with an assigned CancellationToken?
| var credential = await GetCredential(tenant, cancellationToken); | ||
| var loadTestClient = new LoadTestAdministrationClient(new Uri($"https://{dataPlaneUri}"), credential, CreateLoadTestingClientOptions(retryPolicy)); | ||
|
|
||
| var loadTestResponse = await loadTestClient.GetTestAsync(testId); |
There was a problem hiding this comment.
Missing CancellationToken. As with other LoadTestingService.cs comment of mine, should we use the RequestContext parameter with an assigned CancellationToken?
| } | ||
| }; | ||
|
|
||
| var loadTestResponse = await loadTestClient.CreateOrUpdateTestAsync(testId, RequestContent.Create(JsonSerializer.Serialize(testRequestPayload, LoadTestJsonContext.Default.TestRequestPayload))); |
There was a problem hiding this comment.
Missing CancellationToken. As with other LoadTestingService.cs comment of mine, should we use the RequestContext parameter with an assigned CancellationToken?
| @@ -103,7 +103,7 @@ public override async Task<List<string>> GetAvailableRegionsAsync(string resourc | |||
| { | |||
| var quotas = subscription.GetModelsAsync(region); | |||
There was a problem hiding this comment.
Missing use of CancellationToken parameter.
There was a problem hiding this comment.
Even though it's done in the await foreach, I think we should always make sure to use CancellationToken parameters where available.
| @@ -153,7 +153,7 @@ public override async Task<List<string>> GetAvailableRegionsAsync(string resourc | |||
| try | |||
| { | |||
| AsyncPageable<PostgreSqlFlexibleServerCapabilityProperties> result = subscription.ExecuteLocationBasedCapabilitiesAsync(region); | |||
There was a problem hiding this comment.
Also has a CancellationToken parameter.
…erators (microsoft#1649) * add cancellation token usage to enumerator scenarios * update new command instructions to use cancellation token for async enumerable scenario
What does this PR do?
Makes sure a cancellationToken is used whenever an Async Enumerator is traversed using the WithCancellation method. Updated New Command instructions to implement this practice in future generated code
GitHub issue number?
#1583
Pre-merge Checklist
servers/Azure.Mcp.Server/CHANGELOG.mdand/orservers/Fabric.Mcp.Server/CHANGELOG.mdfor product changes (features, bug fixes, UI/UX, updated dependencies)servers/Azure.Mcp.Server/README.mdand/orservers/Fabric.Mcp.Server/README.mddocumentationeng/scripts/Process-PackageReadMe.ps1. See Package README/servers/Azure.Mcp.Server/docs/azmcp-commands.mdand/or/docs/fabric-commands.md.\eng\scripts\Update-AzCommandsMetadata.ps1to update tool metadata in azmcp-commands.md (required for CI)ToolDescriptionEvaluatorand obtained a score of0.4or more and a top 3 ranking for all related test promptsconsolidated-tools.json/servers/Azure.Mcp.Server/docs/e2eTestPrompts.mdcrypto mining, spam, data exfiltration, etc.)/azp run mcp - pullrequest - liveto run Live Test Pipeline