Fix exponential delay behavior (Azure#40869)

* Fix exponential delay behavior

* Update change log

* Fix RetryPolicy call to delay strategy

* update changelog

Author: JoshLove-msft

sdk/core/Azure.Core/CHANGELOG.md

@@ -8,6 +8,9 @@
 
 ### Bugs Fixed
 
+- Fixed exponential retry behavior so that delay milliseconds greater than `Int32.MaxValue` do not trigger an exception.
+- Fixed `DelayStrategy` behavior to no longer shift the delay to be used over by one attempt. Previously, the first delay would be what should have been used for the second, and the second was what should have been used for the third, etc. Note, this would only be observed when using `DelayStrategy` outside of a `RetryPolicy` or `RetryOptions`.
+
 ### Other Changes
 - Remove targets for .NET Core 2.1 and .NET 5 since they are out of support. Azure.Core is no longer compatible with .NET Core 2.1 after removal of target. The remaining targets are unchanged.
 

sdk/core/Azure.Core/src/DelayStrategy.cs

@@ -19,14 +19,15 @@ public abstract class DelayStrategy
         private readonly double _minJitterFactor;
         private readonly double _maxJitterFactor;
         private readonly TimeSpan _maxDelay;
+        internal const double DefaultJitterFactor = 0.2;
 
         /// <summary>
         /// Constructs a new instance of <see cref="DelayStrategy"/>. This constructor can be used by derived classes to customize the jitter factor and max delay.
         /// </summary>
         /// <param name="maxDelay">The max delay value to apply on an individual delay.</param>
         /// <param name="jitterFactor">The jitter factor to apply to each delay. For example, if the delay is 1 second with a jitterFactor of 0.2, the actual
         /// delay used will be a random double between 0.8 and 1.2. If set to 0, no jitter will be applied.</param>
-        protected DelayStrategy(TimeSpan? maxDelay = default, double jitterFactor = 0.2)
+        protected DelayStrategy(TimeSpan? maxDelay = default, double jitterFactor = DefaultJitterFactor)
         {
             // use same defaults as RetryOptions
             _minJitterFactor = 1 - jitterFactor;
@@ -82,7 +83,14 @@ public TimeSpan GetNextDelay(Response? response, int retryNumber) =>
 
         private TimeSpan ApplyJitter(TimeSpan delay)
         {
-            return TimeSpan.FromMilliseconds(_random.Next((int)(delay.TotalMilliseconds * _minJitterFactor), (int)(delay.TotalMilliseconds * _maxJitterFactor)));
+            // get a random double between 0 and 1
+            double randomDouble = _random.NextDouble();
+
+            // scale the double by the jitter range and then add it to the min
+            randomDouble = randomDouble * (_maxJitterFactor - _minJitterFactor) + _minJitterFactor;
+
+            // apply the jitter to the delay
+            return TimeSpan.FromMilliseconds(delay.TotalMilliseconds * randomDouble);
         }
 
         /// <summary>

sdk/core/Azure.Core/src/ExponentialDelayStrategy.cs

@@ -19,6 +19,6 @@ public ExponentialDelayStrategy(
         }
 
         protected override TimeSpan GetNextDelayCore(Response? response, int retryNumber) =>
-            TimeSpan.FromMilliseconds((1 << retryNumber) * _delay.TotalMilliseconds);
+            TimeSpan.FromMilliseconds((1 << (retryNumber - 1)) * _delay.TotalMilliseconds);
     }
-}
+}

sdk/core/Azure.Core/src/Pipeline/RetryPolicy.cs

@@ -264,7 +264,7 @@ private TimeSpan GetNextDelayInternal(HttpMessage message)
         {
             return _delayStrategy.GetNextDelay(
                 message.HasResponse ? message.Response : default,
-                message.RetryNumber);
+                message.RetryNumber + 1);
         }
     }
 }

sdk/core/Azure.Core/tests/ExponentialDelayStrategyTests.cs

@@ -0,0 +1,55 @@
+// Copyright (c) Microsoft Corporation. All rights reserved.
+// Licensed under the MIT License.
+
+using System;
+using Azure.Core.TestFramework;
+using NUnit.Framework;
+
+namespace Azure.Core.Tests
+{
+    public class ExponentialDelayStrategyTests
+    {
+        [Test]
+        public void ExponentialDelayIsWithinBoundsOfJitter()
+        {
+            var initialDelay = TimeSpan.FromSeconds(1);
+            var strategy = DelayStrategy.CreateExponentialDelayStrategy(initialDelay, maxDelay: TimeSpan.MaxValue);
+            var response = new MockResponse(400);
+            for (int retryNumber = 1; retryNumber <= 30; retryNumber++)
+            {
+                AssertDelay(initialDelay, strategy.GetNextDelay(response, retryNumber), retryNumber);
+            }
+        }
+
+        [Test]
+        public void ExponentialDelayRespectsMaxValue()
+        {
+            var initialDelay = TimeSpan.FromSeconds(1);
+            var maxDelay = TimeSpan.FromSeconds(10);
+            var strategy = DelayStrategy.CreateExponentialDelayStrategy(initialDelay, maxDelay: maxDelay);
+            var response = new MockResponse(400);
+            var delay = strategy.GetNextDelay(response, 30);
+            Assert.LessOrEqual(delay.TotalMilliseconds, maxDelay.TotalMilliseconds);
+        }
+
+        [Test]
+        public void ExponentialDelayMillisecondsCanExceedMaxInt()
+        {
+            var initialDelay = TimeSpan.FromSeconds(1);
+            var strategy = DelayStrategy.CreateExponentialDelayStrategy(initialDelay, maxDelay: TimeSpan.MaxValue);
+            var response = new MockResponse(400);
+            AssertDelay(initialDelay, strategy.GetNextDelay(response, 30), 30);
+        }
+
+        private static void AssertDelay(TimeSpan initialDelay, TimeSpan currentDelay, int retryNumber, TimeSpan? maxDelay = default)
+        {
+            var max = maxDelay ?? TimeSpan.MaxValue;
+            Assert.GreaterOrEqual(
+                currentDelay.TotalMilliseconds,
+                Math.Pow((1 - DelayStrategy.DefaultJitterFactor), retryNumber) * (1 << (retryNumber - 1)) * initialDelay.TotalMilliseconds);
+            Assert.LessOrEqual(
+                currentDelay.TotalMilliseconds,
+                Math.Min(max.TotalMilliseconds, Math.Pow(1 + DelayStrategy.DefaultJitterFactor, retryNumber) * (1 << (retryNumber - 1)) * initialDelay.TotalMilliseconds));
+        }
+    }
+}