Add exponential backoff to the retry algorithm of WebCmdlets#26782
Add exponential backoff to the retry algorithm of WebCmdlets#26782mkht wants to merge 7 commits intoPowerShell:masterfrom
Conversation
...t.PowerShell.Commands.Utility/commands/utility/WebCmdlet/Common/WebRequestPSCmdlet.Common.cs
Outdated
Show resolved
Hide resolved
...t.PowerShell.Commands.Utility/commands/utility/WebCmdlet/Common/WebRequestPSCmdlet.Common.cs
Outdated
Show resolved
Hide resolved
|
I've implemented the two feedback points into the code. Since I thought the code readability had decreased, I added comments. |
| // When MaximumRetryCount is not specified, the totalRequests is 1. | ||
| if (totalRequests > 1 && ShouldRetry(response.StatusCode)) | ||
| { | ||
| WebSession.RetryIntervalInSeconds = RetryMode switch |
There was a problem hiding this comment.
WebSession.RetryIntervalInSeconds shouldn't be changed. WebSession is for keeping arguments between calls to the cmdlet.
retryIntervalInSeconds is that we should define before the retry loop and re-calculate here.
Also we could divide by 2 its initial value to avoid extra condition for first pass.
There was a problem hiding this comment.
Are you suggesting adding a new variable within the do-while loop to store the previous retry interval?
There was a problem hiding this comment.
No, if we initialized retryIntervalInSeconds before the retry loop we could retryIntervalInSeconds /= 2.
There was a problem hiding this comment.
Although the initial value is 5 and we will lose precision.
There was a problem hiding this comment.
I see we use retryIntervalInSeconds * 1000 - it is converting to ms. So we still can divide by 2 at init time if we convert to ms in the same time.
int retryIntervalInMilliseconds = WebSession.RetryIntervalInSeconds * 1000 / 2;
There was a problem hiding this comment.
I tested the behavior of curl.
- It always follows the Retry-After header for 429 errors.
- For other errors, the wait time is calculated using an exponential backoff (1sec * 2^n).
Therefore, the logic you suggested to multiply the previous retry time by two differs from curl's approach. I believe reverting to the original logic using MathF.ScaleB() is preferable.
There was a problem hiding this comment.
I don't understand you comment about MathF.ScaleB().
Logic should as previously - if there is Retry-After then use the value, otherwise calculate delay - either fixed or exponential.
There was a problem hiding this comment.
I am discussing the case where multiple retries occur and Retry-After is included in only some of the responses.
The code you proposed, which I implemented in commit dcb8e14 , uses a value equal to twice the previous retry interval as the next wait time.
When Retry-After is not present, this behaves the same as exponential backoff.
However, when the wait time is modified by Retry-After, that value is stored in retryIntervalInSeconds. As a result, the next retry interval becomes twice the Retry-After value.
If Retry-After is not present:
1s > 2s > 4s > 8s
If Retry-After is included only in the response to the second request:
1s > 100s > 200s > 400s
Under the same conditions, curl behaves as follows:
1s > 100s > 4s > 8s
To fix this, we need to modify the current implementation.
One option is to revert the code back to commit 8974732 .
Another option is to introduce a separate variable instead of retryIntervalInSeconds, and avoid storing the Retry-After value in retryIntervalInSeconds.
In short, do it like this.
(Please set aside concerns about float and retryIntervalInMilliseconds for now.)
// ...existing code...
if (totalRequests > 1 && ShouldRetry(response.StatusCode))
{
retryIntervalInSeconds = RetryMode switch
{
// For exponential backoff, we double the retry interval for each retry attempt up to the maximum retry interval.
WebRequestRetryMode.Exponential => Math.Min(retryIntervalInSeconds * 2, _maximumRetryIntervalInSeconds),
WebRequestRetryMode.Fixed or _ => WebSession.RetryIntervalInSeconds
};
// ****Add new temporal variable to store the current wait time.****
int currentRetryIntervalInSeconds = retryIntervalInSeconds;
// If the status code is 429 get the retry interval from the Headers.
// Ignore broken header and its value.
if (response.StatusCode is HttpStatusCode.TooManyRequests && response.Headers.TryGetValues(HttpKnownHeaderNames.RetryAfter, out IEnumerable<string> retryAfter))
{
try
{
IEnumerator<string> enumerator = retryAfter.GetEnumerator();
if (enumerator.MoveNext())
{
// **** Avoid storing the retry interval from the header directly into the retryIntervalInSeconds.****
currentRetryIntervalInSeconds = Convert.ToSingle(enumerator.Current);
}
}
catch
{
// Ignore broken header.
}
}
// ...existing code...There was a problem hiding this comment.
Since now delay is not constant and we evaluate the delay, obviously, we have to store the value and we have to have new variable to take into account Retry-After.
There was a problem hiding this comment.
I have fixed the series of issues that were pointed out.
PR Summary
Add a new parameter
-RetryModetoInvoke-WebRequestandInvoke-RestMethodto add the ability to retry with exponential backoff strategy.The type of
-RetryModeis[WebRequestRetryMode]enum and users can select one from two modes.Fixed: Use fixed retry interval. The interval follows the value of-RetryIntervalSec. This is the default mode.Exponential: Use exponential backoff strategy. The retry interval is expressed asRetryIntervalSec * (2 ^ retryCount).Note 1:
If we select
Exponential, the interval will never exceed 600 seconds. This prevents the interval from becoming excessively long. 600 seconds is used bycurl.Note 2:
This is not a breaking change, since the default mode is
Fixed. It does not change the behavior of existing scripts.PR Context
This PR resolves #19632
PR Checklist
.h,.cpp,.cs,.ps1and.psm1files have the correct copyright headerWIP:or[ WIP ]to the beginning of the title (theWIPbot will keep its status check atPendingwhile the prefix is present) and remove the prefix when the PR is ready.