Merge pull request #509 from microsoft/release/update/250502071537

MattB-msft · web-flow · commit 7bc77a70e7d4 · 2025-05-02T14:02:05.000-07:00
Minor fix for CancellationToken handling between retries
diff --git a/src/GeneralTools/DataverseClient/Client/ConnectionService.cs b/src/GeneralTools/DataverseClient/Client/ConnectionService.cs
@@ -2383,7 +2383,7 @@ internal async Task<HttpResponseMessage> Command_WebExecuteAsync(string queryStr
                         retry = ShouldRetryWebAPI(ex, retryCount, maxRetryCount, retryPauseTime, out isThrottled);
                         if (retry)
                         {
-                            retryCount = await Utilities.RetryRequest(null, requestTrackingId, TimeSpan.Zero, logDt, logEntry, SessionTrackingId, disableConnectionLocking, _retryPauseTimeRunning, ex, errorStringCheck, retryCount, isThrottled, webUriReq: $"{method} {queryString}").ConfigureAwait(false);
+                            retryCount = await Utilities.RetryRequest(null, requestTrackingId, TimeSpan.Zero, logDt, logEntry, SessionTrackingId, disableConnectionLocking, _retryPauseTimeRunning, ex, errorStringCheck, retryCount, isThrottled, webUriReq: $"{method} {queryString}", cancellationToken).ConfigureAwait(false);
                         }
                         else
                         {
diff --git a/src/GeneralTools/DataverseClient/Client/ServiceClient.cs b/src/GeneralTools/DataverseClient/Client/ServiceClient.cs
@@ -1876,17 +1876,23 @@ internal async Task<OrganizationResponse> Command_ExecuteAsyncImpl(OrganizationR
                 catch (Exception ex)
                 {
                     bool isThrottled = false;
-                    retry = ShouldRetry(req, ex, retryCount, out isThrottled);
+                    retry = ShouldRetry(req, ex, retryCount, out isThrottled) || !cancellationToken.IsCancellationRequested;
                     if (retry)
                     {
-                        retryCount = await Utilities.RetryRequest(req, requestTrackingId, LockWait, logDt, _logEntry, SessionTrackingId, _disableConnectionLocking, _retryPauseTimeRunning, ex, errorStringCheck, retryCount, isThrottled).ConfigureAwait(false);
+                        retryCount = await Utilities.RetryRequest(req, requestTrackingId, LockWait, logDt, _logEntry, SessionTrackingId, _disableConnectionLocking, _retryPauseTimeRunning, ex, errorStringCheck, retryCount, isThrottled, cancellationToken: cancellationToken).ConfigureAwait(false);
                     }
                     else
                     {
                         _logEntry.LogRetry(retryCount, req, _retryPauseTimeRunning, true, isThrottled: isThrottled);
                         _logEntry.LogException(req, ex, errorStringCheck);
                         //keep it in end so that LastError could be a better message.
                         _logEntry.LogFailure(req, requestTrackingId, SessionTrackingId, _disableConnectionLocking, LockWait, logDt, ex, errorStringCheck, true);
+
+                        // Callers which cancel should expect to handle a OperationCanceledException
+                        if (ex is OperationCanceledException)
+                            throw;
+                        else
+                            cancellationToken.ThrowIfCancellationRequested();
                     }
                     resp = null;
                 }
@@ -2022,6 +2028,8 @@ private bool ShouldRetry(OrganizationRequest req, Exception ex, int retryCount,
             isThrottlingRetry = false;
             if (retryCount >= _configuration.Value.MaxRetryCount)
                 return false;
+            else if (ex is OperationCanceledException)
+                return false;
             else if (((string.Equals(req.RequestName.ToLower(), "retrieve"))
                 && ((Utilities.ShouldAutoRetryRetrieveByEntityName(((Microsoft.Xrm.Sdk.EntityReference)req.Parameters["Target"]).LogicalName))))
                 || (string.Equals(req.RequestName.ToLower(), "retrievemultiple")
@@ -2047,10 +2055,10 @@ private bool ShouldRetry(OrganizationRequest req, Exception ex, int retryCount,
                     OrgEx.Detail.ErrorCode == ErrorCodes.ThrottlingTimeExceededError ||
                     OrgEx.Detail.ErrorCode == ErrorCodes.ThrottlingConcurrencyLimitExceededError)
                 {
-                     // Use Retry-After delay when specified
+                    // Use Retry-After delay when specified
                     if (OrgEx.Detail.ErrorDetails.TryGetValue("Retry-After", out var retryAfter) && retryAfter is TimeSpan retryAsTimeSpan)
-                    { 
-                         _retryPauseTimeRunning = retryAsTimeSpan;
+                    {
+                        _retryPauseTimeRunning = retryAsTimeSpan;
                     }
                     else
                     {
diff --git a/src/GeneralTools/DataverseClient/Client/Utils/Utils.cs b/src/GeneralTools/DataverseClient/Client/Utils/Utils.cs
@@ -19,6 +19,7 @@
 using System.Net.Http;
 using System.Reflection;
 using System.Text;
+using System.Threading;
 using System.Threading.Tasks;
 #endregion
 
@@ -445,14 +446,20 @@ internal static string ConstructWebApiRequestUrl(OrganizationRequest request, Ht
         /// <param name="retryCount">retryCount</param>
         /// <param name="isThrottled">when set indicated this was caused by a Throttle</param>
         /// <param name="webUriReq"></param>
+        /// <param name="cancellationToken"></param>
         internal static async Task<int> RetryRequest(OrganizationRequest req, Guid requestTrackingId, TimeSpan LockWait, Stopwatch logDt,
             DataverseTraceLogger logEntry, Guid? sessionTrackingId, bool disableConnectionLocking, TimeSpan retryPauseTimeRunning,
-            Exception ex, string errorStringCheck, int retryCount, bool isThrottled, string webUriReq = "")
+            Exception ex, string errorStringCheck, int retryCount, bool isThrottled, string webUriReq = "", CancellationToken cancellationToken = default)
         {
             retryCount++;
             logEntry.LogFailure(req, requestTrackingId, sessionTrackingId, disableConnectionLocking, LockWait, logDt, ex, errorStringCheck, webUriMessageReq: webUriReq);
             logEntry.LogRetry(retryCount, req, retryPauseTimeRunning, isThrottled: isThrottled);
-            await Task.Delay(retryPauseTimeRunning).ConfigureAwait(false);
+
+            // Task.Delay returns early if the CancellationToken is cancelled, but throws a TaskCancelledException if
+            // we pass it an already-cancelled CancellationToken.  If case that occurs, skip the call for consistent behavior.
+            if (!cancellationToken.IsCancellationRequested) 
+                await Task.Delay(retryPauseTimeRunning, cancellationToken).ConfigureAwait(false);
+
             return retryCount;
         }
 
diff --git a/src/GeneralTools/DataverseClient/UnitTests/CdsClient_Core_Tests/ServiceClientTests.cs b/src/GeneralTools/DataverseClient/UnitTests/CdsClient_Core_Tests/ServiceClientTests.cs
@@ -26,6 +26,7 @@
 using System.Net.Http;
 using System.Runtime.CompilerServices;
 using System.Security;
+using System.Threading;
 using System.Threading.Tasks;
 using Xunit;
 using Xunit.Abstractions;
@@ -104,6 +105,23 @@ public void TestThrowDisposedOperationCheck()
             });
         }
 
+        [Fact]
+        public async Task ThrowsOperationCanceledExceptionWhenCancelled()
+        {
+            testSupport.SetupMockAndSupport(out var orgSvc, out var fakHttpMethodHander, out var cli);
+            var cts = new CancellationTokenSource();
+
+            var cancelledToken = cts.Token;
+            cts.Cancel();
+
+            Func<Task> cancelledRequest = async() =>
+            {
+                await cli.ExecuteAsync(new WhoAmIRequest(), cts.Token).ConfigureAwait(false);
+            };
+
+            await cancelledRequest.Should().ThrowAsync<OperationCanceledException>().ConfigureAwait(false);
+        }
+
         [Fact]
         public void ExecuteMessageTests()
         {
@@ -807,6 +825,65 @@ public async void RetryOperationAyncTest()
             Assert.True(retrycount == 2);
         }
 
+        [Fact]
+        public async Task RetryOperationCancelledDuringDelayTest()
+        {
+            testSupport.SetupMockAndSupport(out var orgSvc, out var fakHttpMethodHander, out var cli);
+
+            Guid testGuid = Guid.NewGuid();
+
+            CreateRequest exampleRequest = new CreateRequest();
+            exampleRequest.Target = new Entity("account");
+            exampleRequest.Target.Attributes.Add("id", testGuid);
+
+            Stopwatch testwatch = Stopwatch.StartNew();
+
+            var cts = new CancellationTokenSource();
+            var cancellationToken = cts.Token;
+
+            var delay = TimeSpan.FromSeconds(5);
+            
+            var retryTask = Task.Run(async () =>
+            {
+                await Utilities.RetryRequest(exampleRequest, testGuid, new TimeSpan(0), testwatch, cli._logEntry, null, false, delay, new Exception("Fake_TEST_MSG"), "test retry logic", 0, false, null, cancellationToken: cancellationToken).ConfigureAwait(false);
+            });
+
+            await Task.Delay(TimeSpan.FromMilliseconds(100)).ConfigureAwait(false);
+            cts.Cancel();
+            await Task.Delay(TimeSpan.FromMilliseconds(50)).ConfigureAwait(false);
+
+            retryTask.IsCompleted.Should().BeTrue("Task.Delay within Utilities.RetryRequest should just return early, allowing the task to complete");
+            testwatch.Elapsed.Should().BeLessThan(delay, "Task should return before its delay timer can complete due to cancellation");
+        }
+
+        [Fact]
+        public async Task RetryOperationShouldNotThrowWhenAlreadyCanceledTest()
+        {
+            testSupport.SetupMockAndSupport(out var orgSvc, out var fakHttpMethodHander, out var cli);
+
+            Guid testGuid = Guid.NewGuid();
+
+            CreateRequest exampleRequest = new CreateRequest();
+            exampleRequest.Target = new Entity("account");
+            exampleRequest.Target.Attributes.Add("id", testGuid);
+
+            Stopwatch testwatch = Stopwatch.StartNew();
+
+            var cts = new CancellationTokenSource();
+            var cancellationToken = cts.Token;
+            cts.Cancel(); // Cancel before the token is even provided to the method
+
+            var delay = TimeSpan.FromSeconds(5);
+
+            Func<Task> cancelledRetry = async () =>
+            {
+                await Utilities.RetryRequest(exampleRequest, testGuid, new TimeSpan(0), testwatch, cli._logEntry, null, false, delay, new Exception("Fake_TEST_MSG"), "test retry logic", 0, false, null, cancellationToken: cancellationToken).ConfigureAwait(false);
+            };
+
+            await cancelledRetry.Should().NotThrowAsync<TaskCanceledException>().ConfigureAwait(false);
+            testwatch.Elapsed.Should().BeLessThan(delay, "Task should return before its delay timer can complete due to cancellation");
+        }
+
         #region LiveConnectedTests
 
         [SkippableConnectionTest]
diff --git a/src/nuspecs/Microsoft.PowerPlatform.Dataverse.Client.ReleaseNotes.txt b/src/nuspecs/Microsoft.PowerPlatform.Dataverse.Client.ReleaseNotes.txt
@@ -7,6 +7,9 @@ Notice:
     Note: Only AD on FullFramework, OAuth, Certificate, ClientSecret Authentication types are supported at this time.
 
 ++CURRENTRELEASEID++
+Fix for CancellationToken not canceling retries during delays Git: https://github.com/microsoft/PowerPlatform-DataverseServiceClient/issues/508
+
+1.2.5
 Fix for Null ILogger passed to the AzAuth Client Creators causing a fault.  Git: https://github.com/microsoft/PowerPlatform-DataverseServiceClient/issues/481
 Fix for Memory Leak in Retrieve Multiple. Git: https://github.com/microsoft/PowerPlatform-DataverseServiceClient/issues/463
 Fix for Memory bloat issue caused by not releasing lastErrorMessage property. 

Original file line number	Diff line number	Diff line change
`@@ -2383,7 +2383,7 @@ internal async Task<HttpResponseMessage> Command_WebExecuteAsync(string queryStr`
`2383`	`2383`	`retry = ShouldRetryWebAPI(ex, retryCount, maxRetryCount, retryPauseTime, out isThrottled);`
`2384`	`2384`	`if (retry)`
`2385`	`2385`	`{`
`2386`		`- retryCount = await Utilities.RetryRequest(null, requestTrackingId, TimeSpan.Zero, logDt, logEntry, SessionTrackingId, disableConnectionLocking, _retryPauseTimeRunning, ex, errorStringCheck, retryCount, isThrottled, webUriReq: $"{method} {queryString}").ConfigureAwait(false);`
	`2386`	`+ retryCount = await Utilities.RetryRequest(null, requestTrackingId, TimeSpan.Zero, logDt, logEntry, SessionTrackingId, disableConnectionLocking, _retryPauseTimeRunning, ex, errorStringCheck, retryCount, isThrottled, webUriReq: $"{method} {queryString}", cancellationToken).ConfigureAwait(false);`
`2387`	`2387`	`}`
`2388`	`2388`	`else`
`2389`	`2389`	`{`
Original file line number	Diff line number	Diff line change
`@@ -1876,17 +1876,23 @@ internal async Task<OrganizationResponse> Command_ExecuteAsyncImpl(OrganizationR`
`1876`	`1876`	`catch (Exception ex)`
`1877`	`1877`	`{`
`1878`	`1878`	`bool isThrottled = false;`
`1879`		`- retry = ShouldRetry(req, ex, retryCount, out isThrottled);`
	`1879`	`+ retry = ShouldRetry(req, ex, retryCount, out isThrottled) \|\| !cancellationToken.IsCancellationRequested;`
`1880`	`1880`	`if (retry)`
`1881`	`1881`	`{`
`1882`		`- retryCount = await Utilities.RetryRequest(req, requestTrackingId, LockWait, logDt, _logEntry, SessionTrackingId, _disableConnectionLocking, _retryPauseTimeRunning, ex, errorStringCheck, retryCount, isThrottled).ConfigureAwait(false);`
	`1882`	`+ retryCount = await Utilities.RetryRequest(req, requestTrackingId, LockWait, logDt, _logEntry, SessionTrackingId, _disableConnectionLocking, _retryPauseTimeRunning, ex, errorStringCheck, retryCount, isThrottled, cancellationToken: cancellationToken).ConfigureAwait(false);`
`1883`	`1883`	`}`
`1884`	`1884`	`else`
`1885`	`1885`	`{`
`1886`	`1886`	`_logEntry.LogRetry(retryCount, req, _retryPauseTimeRunning, true, isThrottled: isThrottled);`
`1887`	`1887`	`_logEntry.LogException(req, ex, errorStringCheck);`
`1888`	`1888`	`//keep it in end so that LastError could be a better message.`
`1889`	`1889`	`_logEntry.LogFailure(req, requestTrackingId, SessionTrackingId, _disableConnectionLocking, LockWait, logDt, ex, errorStringCheck, true);`
	`1890`	`+`
	`1891`	`+ // Callers which cancel should expect to handle a OperationCanceledException`
	`1892`	`+ if (ex is OperationCanceledException)`
	`1893`	`+ throw;`
	`1894`	`+ else`
	`1895`	`+ cancellationToken.ThrowIfCancellationRequested();`
`1890`	`1896`	`}`
`1891`	`1897`	`resp = null;`
`1892`	`1898`	`}`
`@@ -2022,6 +2028,8 @@ private bool ShouldRetry(OrganizationRequest req, Exception ex, int retryCount,`
`2022`	`2028`	`isThrottlingRetry = false;`
`2023`	`2029`	`if (retryCount >= _configuration.Value.MaxRetryCount)`
`2024`	`2030`	`return false;`
	`2031`	`+ else if (ex is OperationCanceledException)`
	`2032`	`+ return false;`
`2025`	`2033`	`else if (((string.Equals(req.RequestName.ToLower(), "retrieve"))`
`2026`	`2034`	`&& ((Utilities.ShouldAutoRetryRetrieveByEntityName(((Microsoft.Xrm.Sdk.EntityReference)req.Parameters["Target"]).LogicalName))))`
`2027`	`2035`	`\|\| (string.Equals(req.RequestName.ToLower(), "retrievemultiple")`
`@@ -2047,10 +2055,10 @@ private bool ShouldRetry(OrganizationRequest req, Exception ex, int retryCount,`
`2047`	`2055`	`OrgEx.Detail.ErrorCode == ErrorCodes.ThrottlingTimeExceededError \|\|`
`2048`	`2056`	`OrgEx.Detail.ErrorCode == ErrorCodes.ThrottlingConcurrencyLimitExceededError)`
`2049`	`2057`	`{`
`2050`		`- // Use Retry-After delay when specified`
	`2058`	`+ // Use Retry-After delay when specified`
`2051`	`2059`	`if (OrgEx.Detail.ErrorDetails.TryGetValue("Retry-After", out var retryAfter) && retryAfter is TimeSpan retryAsTimeSpan)`
`2052`		`- {`
`2053`		`- _retryPauseTimeRunning = retryAsTimeSpan;`
	`2060`	`+ {`
	`2061`	`+ _retryPauseTimeRunning = retryAsTimeSpan;`
`2054`	`2062`	`}`
`2055`	`2063`	`else`
`2056`	`2064`	`{`