Add exponential backoff to the retry algorithm of WebCmdlets#26782
Add exponential backoff to the retry algorithm of WebCmdlets#26782mkht wants to merge 6 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.
Moved the initialization of retryIntervalInSeconds outside the loop and simplified the backoff calculation logic.
Also, I changed the type of retryIntervalInSeconds from int to float to prevent loss of precision.
There was a problem hiding this comment.
I noticed an issue in one edge case.
The logic that doubles the retry interval after each previous retry may conflict with the logic that always prioritizes the Retry-After header value for 429 errors.
Consider a scenario where the user sets RetryIntervalSec to 4 and MaximumRetryCount to 3. Suppose the remote endpoint returns errors in the following sequence for three requests:
- 409 Conflict
- 429 Too Many Requests (Retry-After: 100 sec)
- 409 Conflict
The actual retry intervals under the current logic are:
4 sec > 100 sec > 200 sec
This may differ from the user's expectation.
The user likely expects:
4 sec > 100 sec > 16 sec
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.